[RFC: Patch, PR 60102] [4.9/4.10 Regression] powerpc fp-bit ices@dwf_regno

[Ulrich Weigand]

Rohit wrote:

> This is related to the following bug:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D60102
> 
> I have tried to fix the e500v2 build on GCC v4.9.0 with the attached patch.
> Can you please review and comment on the changes especially DWARF_FRAME_REG=
> NUM, DWARF_REG_TO_UNWIND_COLUMN definitions?

David asked me to comment on the use of DWARF register numbers in this patch.

There's a number of register number "address spaces" in play here:

(A) GCC hard register numbers
(B) DWARF register numbers used in .debug_info etc.
(C) DWARF CFI register numbers (GCC internal)
(D) DWARF CFI register numbers as used in .debug_frame
(E) DWARF CFI register numbers as used in .eh_frame
(F) DWARF CFI unwind column numbers

These are a number of macros to convert between them:

DBX_REGISTER_NUMBER: (A) -> (B)
DWARF_FRAME_REGNUM: (A) -> (C)
DWARF2_FRAME_REG_OUT: (C) -> (D) / (E)
DWARF_REG_TO_UNWIND_COLUMN: (E) -> (F)


Note that some of these seem to be used incorrectly in current rs6000.c:

      for (i = FIRST_ALTIVEC_REGNO; i < LAST_ALTIVEC_REGNO+1; i++)
        {
          int column = DWARF_REG_TO_UNWIND_COLUMN (i);
          HOST_WIDE_INT offset
            = DWARF_FRAME_REGNUM (column) * GET_MODE_SIZE (mode);

This should rather be

          int column = DWARF_REG_TO_UNWIND_COLUMN (DWARF_FRAME_REGNUM (i));
          HOST_WIDE_INT offset = column * GET_MODE_SIZE (mode);

which doesn't show up as problem currently since DWARF_FRAME_REGNUM
is defined as the identity mapping, but will show up once you have to
actually define a nontrivial mapping in DWARF_FRAME_REGNUM.

[ To be fully correct, I guess it actually should be

  int column = DWARF_REG_TO_UNWIND_COLUMN
                (DWARF2_FRAME_REG_OUT (DWARF_FRAME_REGNUM (i), true));

  but DWARF2_FRAME_REG_OUT (..., true) is the identity map as well ... ]


Now, if I understand the SPE situation correctly, you had previously:

- no GCC hard register numbers
  (however, rs6000_dwarf_register_span, which is supposed to return a
  hard register number, returned numbers in the 1200..1231 range)
- used the 1200..1231 range for (B), (C), (D), and (E)
- used the 113..145 range for (F)

Now, you need to introduce new GCC hard register numbers (A).  However, in
order to preserve compatibility with DWARF info in existing binaries, none
of (B), (D), (E) or (F) is allowed to change.  [ (C) could change in theory,
but it's probably best not to change it either.  ]

Your patch now defines the new GCC hard register numbers in the 117..149 range,
which seems reasonable.  However, you ought to the leave the other mappings
unchanged.  For (B) this looks OK due to the rs6000_dbx_register_number change.

However (C), (D), and (E) *do* change with your patch:

> -#define DWARF_FRAME_REGNUM(REGNO) (REGNO)
> +#define DWARF_FRAME_REGNUM(REGNO) \
> +  ((REGNO) >= 1200 ? ((REGNO) - 1200 + (DWARF_FRAME_REGISTERS - 32)) : (REGNO))

This isn't OK; the input to DWARF_FRAME_REGNUM is a GCC hard register number,
which will never be in the 1200... range.

On the other hand, you can now get hard register numbers in the 117..149 range,
which you need to map *back* to the 1200..1231 range, or else CFI register
numbers will be wrong.  So you should have something like:

#define DWARF_FRAME_REGNUM(REGNO) \
  (SPE_HIGH_REGNO_P(REGNO)? ((REGNO) - FIRST_SPE_HIGH_REGNO + 1200) : (REGNO))

On the other hand, the DWARF_REG_TO_UNWIND_COLUMN macro needs to map that
1200..1231 range back to the 113..145 range, so it should just stay as-is.

Note that (F) ends up being OK with your patch as-is, since the two bugs
in DWARF_FRAME_REGNUM and DWARF_REG_TO_UNWIND_COLUMN cancel each other out.


A couple of further comments on the patch:

> Index: libgcc/config/rs6000/linux-unwind.h
> ===================================================================
> --- libgcc/config/rs6000/linux-unwind.h	(revision 212339)
> +++ libgcc/config/rs6000/linux-unwind.h	(working copy)
> @@ -274,8 +274,8 @@ ppc_fallback_frame_state (struct _Unwind
>  #ifdef __SPE__
>    for (i = 14; i < 32; i++)
>      {
> -      fs->regs.reg[i + FIRST_PSEUDO_REGISTER - 1].how = REG_SAVED_OFFSET;
> -      fs->regs.reg[i + FIRST_PSEUDO_REGISTER - 1].loc.offset
> +      fs->regs.reg[i + FIRST_SPE_HIGH_REGNO - 4].how = REG_SAVED_OFFSET;
> +      fs->regs.reg[i + FIRST_SPE_HIGH_REGNO - 4].loc.offset

This is a change to current behaviour, but that was probably intended
since the old behaviour seems broken (apparently wasn't updated after
the introduction of the three HTM registers).

> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c	(revision 212339)
> +++ gcc/config/rs6000/rs6000.c	(working copy)
> @@ -30956,7 +30956,7 @@ rs6000_init_dwarf_reg_sizes_extra (tree 
>        rtx mem = gen_rtx_MEM (BLKmode, addr);
>        rtx value = gen_int_mode (4, mode);
>  
> -      for (i = 1201; i < 1232; i++)
> +      for (i = FIRST_SPE_HIGH_REGNO; i < LAST_SPE_HIGH_REGNO+1; i++)

Again this seems to change behaviour, but the old seems broken (didn't
initialize the first SPE high register).

> -/* Add 32 dwarf columns for synthetic SPE registers.  */
> -#define DWARF_FRAME_REGISTERS ((FIRST_PSEUDO_REGISTER - 4) + 32)
> +/* SPE high registers added as hard regs. 
> +   The 3 HTM registers aren't included in DWARF_FRAME_REGISTERS */
> +#define DWARF_FRAME_REGISTERS (FIRST_PSEUDO_REGISTER - 4)

This is OK, but the comment is confusing: the -4 is because *four*
registers aren't included in DWARF_FRAME_REGISTER, namely the 3 HTM
registers *and the sfp register*.

>  /* The SPE has an additional 32 synthetic registers, with DWARF debug
>     info numbering for these registers starting at 1200.  While eh_frame
> @@ -951,13 +952,14 @@ enum data_align { align_abi, align_opt, 
>     We must map them here to avoid huge unwinder tables mostly consisting
>     of unused space.  */
>  #define DWARF_REG_TO_UNWIND_COLUMN(r) \
> -  ((r) > 1200 ? ((r) - 1200 + (DWARF_FRAME_REGISTERS - 32)) : (r))
> +  ((r) >= FIRST_SPE_HIGH_REGNO ? ((r) - FIRST_SPE_HIGH_REGNO + (DWARF_FRAME_REGISTERS - 32)) : (r)) 

As discussed above, this shouldn't change.

>  /* Use gcc hard register numbering for eh_frame.  */
> -#define DWARF_FRAME_REGNUM(REGNO) (REGNO)
> +#define DWARF_FRAME_REGNUM(REGNO) \
> +  ((REGNO) >= 1200 ? ((REGNO) - 1200 + (DWARF_FRAME_REGISTERS - 32)) : (REGNO))

As discussed above, this is wrong.

> +  { 0x00000000, 0x00000000, 0x00000000, 0xfff00000, 0x000fffff }, /* SPE_HIGH_REGS */    \
> +  { 0xffffffff, 0xffffffff, 0xfffffffe, 0xfff7ffff, 0x000fffff }  /* ALL_REGS */         \

This looks wrong to me; the SPE high regs have hard register numbers
in the 117..149 range.  117 is not a multiple of 4, so there cannot
be just "f" hex characters in the map for SPE_HIGH_REGS.


> +  &rs6000_reg_names[117][0],	/* SPE rh0   */				\
> +  &rs6000_reg_names[118][0],	/* SPE rh1   */				\
> +  &rs6000_reg_names[119][0],	/* SPE rh2   */				\

You need to actually initialize those rs6000_reg_names field in rs6000.c
if you refer to them here.


Bye,
Ulrich


-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com