[RFC] When adding an offset to a lo_sum containing a symbol, check it is in range of the symbol's alignment

[Richard Henderson]

On 04/04/2016 05:36 AM, Andrew Bennett wrote:
> Hi,
>
> In MIPS (and similarly for other RISC architectures) to load an absolute address of an object
> requires a two instruction sequence: one instruction to load the high part of the object's address,
> and one instruction to load the low part of the object's address.  Typically the result from the
> calculation of the high part of the address will only be used by one instruction to load
> the low part of the address.  However, in certain situations (for example when loading or
> storing double word values) the result of computing the high part of the address can be
> used by multiple instructions to load the low parts of an address at different offsets.  Lets
> show this with an example C program.
>
> struct
> {
>    short s;
>    unsigned long long l;
> } h;
>
> void foo (void)
> {
>    h.l = 0;
> }
>
> When this is compiled for MIPS it produces the following assembly:
>
>          lui     $2,%hi(h+8)
>          sw      $0,%lo(h+8)($2)
>          jr      $31
>          sw      $0,%lo(h+12)($2)
>
> 	  ...
>
>          .globl  h
>          .section        .bss,"aw",@nobits
>          .align  3
>          .type   h, @object
>          .size   h, 16
> h:
>          .space  16
>
>
> Notice here that the high part of the address of object h is loaded into register $2,
> and this is then used as part of the low part calculation by two the sw instructions which each
> have different offsets.  In MIPS the value of a low part calculation is treated as a signed value.
> It is therefore valid to use the result of a high part calculation with multiple low part calculations
> containing different offsets so long as when adding the result of the high part to the each of the
> sign extended low parts we get valid addresses.
>
> However, if we add the packed attribute to the h structure, the fields of the structure will
> not be naturally aligned and we can break the previous condition.  Lets explain this in more
> detail with the following C program.
>
> struct __attribute__((packed))
> {
>   short s;
>   unsigned long long l;
> } h;
>
> void foo (void)
> {
>   h.l = 0;
> }
>
> When this is compiled for MIPS it produces the following assembly:
>
> 	  lui     $2,%hi(h)
>          addiu   $4,$2,%lo(h+2)
>          addiu   $3,$2,%lo(h+6)
>          swl     $0,3($4)
>          swr     $0,%lo(h+2)($2)
>          swl     $0,3($3)
>          jr      $31
>          swr     $0,%lo(h+6)($2)
>
>          ...
>
> 	  .globl  h
>          .section        .bss,"aw",@nobits
>          .align  2
>          .type   h, @object
>          .size   h, 10
> h:
>          .space  10
>
>
> There are two things to highlight here.  Firstly the alignment of the h structure has decreased
> from 8 bytes to 4 bytes.  Secondly we have a low part calculation which adds an offset of 6
> to the address of the h structure which is greater than its alignment.
>
> When the MIPS linker resolves a HI relocation (i.e. %hi(h)) it finds the next LO
> relocation (i.e. %lo(h+2)) in the relocation table and using the information from both
> of these relocations it computes the object's address and extracts its high part.  Then, when the
> MIPS linker resolves a LO relocation it adds the offset to the object's address and then extracts
> the low part.
>
> Lets assume that object h has an address of 0x80007ffc.  When the MIPS linker resolves the value
> of the HI relocation for object h, it will also use the value of the LO relocation for object h
> with an offset of 2.  The high part value is therefore:
>
> HIGH (0x80007ffc + 2) = HIGH (0x80007ffe) = 0x8000
>
>
> Then the MIPS linker resolves the value of LO relocation for object h with an offset of 2:
>
> LO (0x80007ffc + 2) = LO (0x80007ffe) = 0x7ffe
>
>
> Finally the MIPS linker resolves the value of the LO relocation for object h with an offset of 6:
>
> LO (0x80007ffc + 6) = LO (0x80008002) = 0x8002
>
> In MIPS the value of a LO relocation is treated as a signed value, so when the program is run the address
> of h+6 will be 0x7fff8002 when it should be 0x80008002.
>
>
> The place in the GCC middle end where the invalid RTX is generated is in the adjust_address_1 function
> in emit-rtl.c, when it adjusts the offset of an address which is a MEM rtx containing a LO_SUM,
> by producing a new LO_SUM containing the offset.  In the case where the original LO_SUM contains a SYMBOL_REF
> this is only valid if the alignment of the object is less than the offset.
>
> The potential fix (shown below) is to perform this check.  Before I produce a formal patch and testcase
> for this issue I wanted to discuss it with the GCC community to ensure this is the best place to fix such
> a problem.
>
> So, if required please feel free to suggest improvements or other more appropriate places to apply this fix.
>
>
> Many thanks,
>
>
>
> Andrew
>
>
>
> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index 3041b9e..b20de6f 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -2036,12 +2036,30 @@ adjust_address_1 (rtx memref, enum machine_mode mode, HOST_WIDE_INT offset,
>
>     if (adjust_address)
>       {
> +      unsigned HOST_WIDE_INT symbol_alignment;
> +
> +      symbol_alignment = GET_MODE_ALIGNMENT (GET_MODE (memref)) / BITS_PER_UNIT;
> +
> +      if (GET_CODE (addr) == LO_SUM)
> +       {
> +         rtx sym = XEXP (addr, 1);
> +
> +         if (GET_CODE (sym) == CONST
> +             && GET_CODE (XEXP (sym, 0)) == PLUS)
> +           sym = XEXP (XEXP (sym, 0), 0);

There's probably an offset here with the plus...

> +
> +         if (GET_CODE (sym) == SYMBOL_REF
> +             && SYMBOL_REF_DECL (sym))
> +           symbol_alignment = DECL_ALIGN_UNIT (SYMBOL_REF_DECL (sym));
> +       }
> +
>         /* If MEMREF is a LO_SUM and the offset is within the alignment of the
>           object, we can merge it into the LO_SUM.  */
>         if (GET_MODE (memref) != BLKmode && GET_CODE (addr) == LO_SUM
>            && offset >= 0
> -         && (unsigned HOST_WIDE_INT) offset
> +         && ((unsigned HOST_WIDE_INT) offset
>                < GET_MODE_ALIGNMENT (GET_MODE (memref)) / BITS_PER_UNIT)
> +         && ((unsigned HOST_WIDE_INT) offset < symbol_alignment))

... that you're not considering here.  Which suggests that multiple additions 
will succeed when we're well past the alignment of the symbol.

I'm a bit surprised that we're not already using attrs.offset_known_p and 
attrs.align here.  Surely that existing information could be of use...


r~