[PATCH] New ia64 @slotcount pseudo func (3rd try)

[Douglas B Rupp]

There's been alot of comments on this patch and I appreciate everyone's
time in making them. I tried to address all the new concerns, except for
the use of ',' vs '-' in the expression arguments. IMO is looks fine
either way, and the Gcc side of the patch has been approved with '-'.
If someone feels strongly about the issue, I can always resubmit the Gcc
patch.



2009-11-04  Douglas B Rupp  <rupp@...>

        * gas/config/tc-ia64.c (PSEUDO_FUNC_EXPR): New pseudo_func type.
        (@slotcount): New pseudo func of type EXPR.
        (ia64_parse_name): Implement @slotcount.

--- gas/config/tc-ia64.c 2009-10-06 22:13:53.000000000 -0700
+++ gas/config/tc-ia64.c 2009-11-04 11:19:52.286974547 -0800
@@ -545,7 +545,8 @@ static struct
  PSEUDO_FUNC_RELOC,
  PSEUDO_FUNC_CONST,
  PSEUDO_FUNC_REG,
- PSEUDO_FUNC_FLOAT
+ PSEUDO_FUNC_FLOAT,
+ PSEUDO_FUNC_EXPR
       }
     type;
     union
@@ -576,6 +577,11 @@ pseudo_func[] =
     { NULL, 0, { 0 } }, /* placeholder for FUNC_LT_TP_RELATIVE */
     { "iplt", PSEUDO_FUNC_RELOC, { 0 } },
 
+#ifdef TE_VMS
+    /* expression pseudo functions:  */
+    { "slotcount", PSEUDO_FUNC_EXPR, { 0 } },
+#endif
+
     /* mbtype4 constants:  */
     { "alt", PSEUDO_FUNC_CONST, { 0xa } },
     { "brcst", PSEUDO_FUNC_CONST, { 0x0 } },
@@ -7779,6 +7785,93 @@ ia64_parse_name (char *name, expressionS
   *nextcharP = *input_line_pointer;
   break;
 
+#ifdef TE_VMS
+ case PSEUDO_FUNC_EXPR:
+  if (*nextcharP != '(')
+    {
+      as_bad (_("Expected '('"));
+      break;
+    }
+  /* Skip '('.  */
+  *input_line_pointer++ = '(';
+
+  /* Whitespace normally has been removed already here let's be
+     paranoid, e.g. this function could be invoked from inside GDB.  */
+  SKIP_WHITESPACE ();
+
+  /* The only expression pseudo func at this time is @slotcount
+     which takes an expression like (beg-end), where
+     beg and end are addresses, and figures out how many
+     Itanium instruction slots separate the addresses.  The value
+     is needed for a couple of VMS debugger attributes relating to
+     prologue and epilogue size.  */
+
+  {
+    char *name;
+    char c;
+    symbolS *symbolPend, *symbolPbeg;
+    int end, beg, val;
+    bfd_vma frag_off;
+
+    name = input_line_pointer;
+    c = get_symbol_end ();
+    symbolPend = symbol_find (name);
+    *input_line_pointer = c;
+
+    SKIP_WHITESPACE ();
+    if (*input_line_pointer++ != '-')
+    {
+      c = *--input_line_pointer;
+      *input_line_pointer = '\0';
+      as_bad (_("expected \"-\" after \"%s\" found \"%c\""), name, c);
+      *input_line_pointer++ = c;
+      *nextcharP = *input_line_pointer;
+      return 0;
+    }
+    SKIP_WHITESPACE ();
+
+    name = input_line_pointer;
+    c = get_symbol_end ();
+    symbolPbeg = symbol_find (name);
+    *input_line_pointer = c;
+
+    if (symbolPend == NULL
+        || symbolPbeg == NULL
+        || !S_IS_DEFINED (symbolPend)
+        || !S_IS_DEFINED (symbolPbeg)
+        || S_GET_SEGMENT (symbolPend) != S_GET_SEGMENT (symbolPbeg)
+        || !frag_offset_fixed_p (symbol_get_frag (symbolPend),
+         symbol_get_frag (symbolPbeg),
+         &frag_off))
+      {
+         as_bad ("invalid @slotcount expression");
+        *nextcharP = *input_line_pointer;
+        return 0;
+      }
+
+    /* Calculate the number of instruction slots between the two
+       labels. Adjust for frag_off, at this stage of assembly, symbol
+       values are offsets within their frags.  The two symbols may be
+       in different frags.  */
+    end = S_GET_VALUE (symbolPend) - frag_off;
+    beg = S_GET_VALUE (symbolPbeg);
+
+    val = (((end & -16) - (beg & -16)) / 16 * 3)
+  + (end & 15)
+  - (beg & 15);
+
+    e->X_op = O_constant;
+    e->X_add_number = val;
+  }
+
+  SKIP_WHITESPACE ();
+  if (*input_line_pointer != ')')
+    as_bad (_("Missing ')'"));
+
+  *nextcharP = *++input_line_pointer;
+  break;
+#endif
+
  case PSEUDO_FUNC_CONST:
   e->X_op = O_constant;
   e->X_add_number = pseudo_func[idx].u.ival;

[Nick Clifton]

Hi Douglas,

> There's been alot of comments on this patch and I appreciate everyone's
> time in making them.

Guess what - I have a few more, very minor, changes that I would like to
see in the patch.  These are mostly formatting issues, but it would be
nice to see them fixed:

   +#ifdef TE_VMS
   +    /* expression pseudo functions:  */

Comment formatting - the comment should be treated as a sentence and
start with a capital letter.  (I do realize that the other comments in
this part of the code are formatted in the same way as your comment, but
they are also wrong and should be fixed one day).

   +    if (*input_line_pointer++ != '-')
   +    {

Indentation - the opening curly brace, (and the code that follows),
should be indented by two more spaces.

   +         as_bad ("invalid @slotcount expression");

Internationalization - the error message should be enclosed in _(....).

   +    val = (((end & -16) - (beg & -16)) / 16 * 3)
   +  + (end & 15)
   +  - (beg & 15);

Just a suggestion here - this expression seems rather complex and
non-intuitive.  Maybe a comment describing how the computation works
would be useful for future readers of the code.

Cheers
   Nick

[Douglas B Rupp]

Thanks for the formatting suggestions. I should have caught most of them
beforehand, sorry. See attached for review.

--Douglas Rupp
AdaCore

2009-11-05  Douglas B Rupp  <rupp@...>

        * gas/config/tc-ia64.c (PSEUDO_FUNC_EXPR): New pseudo_func type.
        (@slotcount): New pseudo func of type EXPR.
        (ia64_parse_name): Implement @slotcount.

--- gas/config/tc-ia64.c 2009-10-06 22:13:53.000000000 -0700
+++ gas/config/tc-ia64.c 2009-11-05 10:34:53.707197018 -0800
@@ -545,7 +545,8 @@ static struct
  PSEUDO_FUNC_RELOC,
  PSEUDO_FUNC_CONST,
  PSEUDO_FUNC_REG,
- PSEUDO_FUNC_FLOAT
+ PSEUDO_FUNC_FLOAT,
+ PSEUDO_FUNC_EXPR
       }
     type;
     union
@@ -576,6 +577,11 @@ pseudo_func[] =
     { NULL, 0, { 0 } }, /* placeholder for FUNC_LT_TP_RELATIVE */
     { "iplt", PSEUDO_FUNC_RELOC, { 0 } },
 
+#ifdef TE_VMS
+    /* Expression pseudo functions:  */
+    { "slotcount", PSEUDO_FUNC_EXPR, { 0 } },
+#endif
+
     /* mbtype4 constants:  */
     { "alt", PSEUDO_FUNC_CONST, { 0xa } },
     { "brcst", PSEUDO_FUNC_CONST, { 0x0 } },
@@ -7779,6 +7785,99 @@ ia64_parse_name (char *name, expressionS
   *nextcharP = *input_line_pointer;
   break;
 
+#ifdef TE_VMS
+ case PSEUDO_FUNC_EXPR:
+  if (*nextcharP != '(')
+    {
+      as_bad (_("Expected '('"));
+      break;
+    }
+  /* Skip '('.  */
+  *input_line_pointer++ = '(';
+
+  /* Whitespace normally has been removed already here let's be
+     paranoid, e.g. this function could be invoked from inside GDB.  */
+  SKIP_WHITESPACE ();
+
+  /* The only expression pseudo func at this time is @slotcount
+     which takes an expression like (beg-end), where
+     beg and end are addresses, and figures out how many
+     Itanium instruction slots separate the addresses.  The value
+     is needed for a couple of VMS debugger attributes relating to
+     prologue and epilogue size.  */
+
+  {
+    char *name;
+    char c;
+    symbolS *symbolPend, *symbolPbeg;
+    int end, beg, val;
+    bfd_vma frag_off;
+
+    name = input_line_pointer;
+    c = get_symbol_end ();
+    symbolPend = symbol_find (name);
+    *input_line_pointer = c;
+
+    SKIP_WHITESPACE ();
+    if (*input_line_pointer++ != '-')
+      {
+ c = *--input_line_pointer;
+ *input_line_pointer = '\0';
+ as_bad (_("expected \"-\" after \"%s\" found \"%c\""),
+ name, c);
+ *input_line_pointer++ = c;
+ *nextcharP = *input_line_pointer;
+ return 0;
+      }
+    SKIP_WHITESPACE ();
+
+    name = input_line_pointer;
+    c = get_symbol_end ();
+    symbolPbeg = symbol_find (name);
+    *input_line_pointer = c;
+
+    if (symbolPend == NULL
+        || symbolPbeg == NULL
+        || !S_IS_DEFINED (symbolPend)
+        || !S_IS_DEFINED (symbolPbeg)
+        || S_GET_SEGMENT (symbolPend) != S_GET_SEGMENT (symbolPbeg)
+        || !frag_offset_fixed_p (symbol_get_frag (symbolPend),
+         symbol_get_frag (symbolPbeg),
+         &frag_off))
+      {
+         as_bad (_("invalid @slotcount expression"));
+        *nextcharP = *input_line_pointer;
+        return 0;
+      }
+
+    /* Adjust for frag_off, at this stage of assembly, symbol values
+       are offsets within their frags.  The two symbols may be in
+       different frags.  */
+    end = S_GET_VALUE (symbolPend) - frag_off;
+    beg = S_GET_VALUE (symbolPbeg);
+
+    /* IA64 instruction bundles are 16 bytes and contain 3 instructions
+       each.  Given that the beg and end labels are debugging labels
+       they may not be on bundle boundaries, so finding the slotcount
+       difference consists of computing the bundle seperation in slots
+       and adjusting for the offset of the beg and end labels within
+       the bundles.  */
+    val = (((end & -16) - (beg & -16)) / 16 * 3)
+  + (end & 15)
+  - (beg & 15);
+
+    e->X_op = O_constant;
+    e->X_add_number = val;
+  }
+
+  SKIP_WHITESPACE ();
+  if (*input_line_pointer != ')')
+    as_bad (_("Missing ')'"));
+
+  *nextcharP = *++input_line_pointer;
+  break;
+#endif
+
  case PSEUDO_FUNC_CONST:
   e->X_op = O_constant;
   e->X_add_number = pseudo_func[idx].u.ival;

[Douglas B Rupp]

Some problems have appeared. I should have run more complex tests.

frag_offset_fixed_p is returning false for some cases involving the
begin epilogue label slotcount from the begin function label

         data4.ua        @slotcount(.LEB56-.LFB56)

-------------------
(gdb) print frag1
$21 = (const fragS *) 0x766890
(gdb) print frag2
$22 = (const fragS *) 0x7667c8
(gdb) n
407       frag = frag1;
(gdb) n
408       while (frag->fr_type == rs_fill)
(gdb) print frag->fr_type
$23 = rs_align_code
(gdb) n
422       off = frag1->fr_address - frag2->fr_address;
(gdb) print frag->fr_type
$24 = rs_align_code
(gdb) print off
$25 = 0
(gdb) n
423       frag = frag2;
(gdb)
424       while (frag->fr_type == rs_fill)
(gdb) print frag->fr_type
$26 = rs_align_code
(gdb) n
437       return FALSE;

[Douglas B Rupp]

Seems like from this comment rs_align_code could be handled in
frag_offset_fixed_p, no?

   /* Align code.  The fr_offset field holds the power of 2 to which
      to align.  This type is only generated by machine specific
      code, which is normally responsible for handling the fill
      pattern.  The fr_subtype field holds the maximum number of
      bytes to skip when aligning, or 0 if there is no maximum.  */
   rs_align_code,


Douglas B Rupp wrote:

> Some problems have appeared. I should have run more complex tests.
>
> frag_offset_fixed_p is returning false for some cases involving the
> begin epilogue label slotcount from the begin function label
>
>         data4.ua        @slotcount(.LEB56-.LFB56)
>
> -------------------
> (gdb) print frag1
> $21 = (const fragS *) 0x766890
> (gdb) print frag2
> $22 = (const fragS *) 0x7667c8
> (gdb) n
> 407       frag = frag1;
> (gdb) n
> 408       while (frag->fr_type == rs_fill)
> (gdb) print frag->fr_type
> $23 = rs_align_code
> (gdb) n
> 422       off = frag1->fr_address - frag2->fr_address;
> (gdb) print frag->fr_type
> $24 = rs_align_code
> (gdb) print off
> $25 = 0
> (gdb) n
> 423       frag = frag2;
> (gdb)
> 424       while (frag->fr_type == rs_fill)
> (gdb) print frag->fr_type
> $26 = rs_align_code
> (gdb) n
> 437       return FALSE;
>

[Douglas B Rupp]

I ran objdump -d on a version of the object file (unwind-ia64.o) from
before we started down slotcounts in the assembler, calculated the
slotcount by hand and it came out right in this case with a frag_off of
zero, even though the labels are clearly in different frags, so now I'm
really confused. Giving up until someone tells me something.

Douglas B Rupp wrote:

> Seems like from this comment rs_align_code could be handled in
> frag_offset_fixed_p, no?
>
>   /* Align code.  The fr_offset field holds the power of 2 to which
>      to align.  This type is only generated by machine specific
>      code, which is normally responsible for handling the fill
>      pattern.  The fr_subtype field holds the maximum number of
>      bytes to skip when aligning, or 0 if there is no maximum.  */
>   rs_align_code,
>
>
> Douglas B Rupp wrote:
>> Some problems have appeared. I should have run more complex tests.
>>
>> frag_offset_fixed_p is returning false for some cases involving the
>> begin epilogue label slotcount from the begin function label
>>
>>         data4.ua        @slotcount(.LEB56-.LFB56)
>>
>> -------------------
>> (gdb) print frag1
>> $21 = (const fragS *) 0x766890
>> (gdb) print frag2
>> $22 = (const fragS *) 0x7667c8
>> (gdb) n
>> 407       frag = frag1;
>> (gdb) n
>> 408       while (frag->fr_type == rs_fill)
>> (gdb) print frag->fr_type
>> $23 = rs_align_code
>> (gdb) n
>> 422       off = frag1->fr_address - frag2->fr_address;
>> (gdb) print frag->fr_type
>> $24 = rs_align_code
>> (gdb) print off
>> $25 = 0
>> (gdb) n
>> 423       frag = frag2;
>> (gdb)
>> 424       while (frag->fr_type == rs_fill)
>> (gdb) print frag->fr_type
>> $26 = rs_align_code
>> (gdb) n
>> 437       return FALSE;
>>
>

[Alan Modra]

On Thu, Nov 05, 2009 at 01:48:52PM -0800, Douglas B Rupp wrote:
> Seems like from this comment rs_align_code could be handled in  
> frag_offset_fixed_p, no?
>
>   /* Align code.  The fr_offset field holds the power of 2 to which
>      to align.  This type is only generated by machine specific
>      code, which is normally responsible for handling the fill
>      pattern.  The fr_subtype field holds the maximum number of
>      bytes to skip when aligning, or 0 if there is no maximum.  */
>   rs_align_code,

No, it can't be.  An alignment frag means that the offset between that
frag and following frags isn't known until the final address of the
alignment frag is known.  Alignment inserts a variable sized pad.

This means that you cannot resolve the expression down to O_constant
in ia64_parse_name.  Instead you'll need to build an expression using
O_md1 (if that isn't already used by ia64) and the two symbols, and
handle it in md_apply_fix.  And handle any other fallout.

--
Alan Modra
Australia Development Lab, IBM