Harden rs6000 offsettable_ok_by_alignment

Prior to a fix I committed recently on the gcc-4.5-ibm branch, we used
to fail with a linker error: "relocation R_PPC64_TOC16_LO_DS not a
multiple of 4" on code loading the "md_backup_data-1" string in the
following testcase.

typedef __signed__ char __s8;
typedef unsigned char __u8;
typedef __signed__ short __s16;
typedef unsigned short __u16;
typedef __signed__ int __s32;
typedef unsigned int __u32;
typedef __signed__ long __s64;
typedef unsigned long __u64;

static struct mdp_backup_super {
 char magic[16];
 __u8 set_uuid[16];
 __u64 mtime;

 __u64 devstart;
 __u64 arraystart;
 __u64 length;
 __u32 sb_csum;
 __u32 pad1;
 __u64 devstart2;
 __u64 arraystart2;
 __u64 length2;
 __u32 sb_csum2;
 __u8 pad[512-68-32];
} __attribute__((aligned(512))) bsb;

void f (void)
{
  __builtin_memcpy (bsb.magic, "md_backup_data-1", 16);
}

The memcpy gets optimised to two ld insns followed by two std insns,
with both the address of bsb and the string being toc-relative.
bsb.magic is sufficiently aligned for the stores to be OK, but the
string is not aligned at all;  The loads may well be at an address
that is not a multiple of four.  It is also quite possible for the
string to straddle a 32k boundary, resulting in rubbish being loaded
in the second dword.

On mainline, the situation is a little different due to changes in
memref handling.  I seem to always get a VAR_DECL for the string,
correctly reporting an alignment of one, so we don't use the invalid
toc-relative address.  ie. I can't find a testcase for this potential
problem on mainline.  What worries me is that this may simply be due
to not trying enough compiler options.  Also, future changes in gcc
might result in STRING_CST trees appearing as they do on 4.5.  So I'd
like to apply the following patch.  Bootstrapped etc. powerpc64-linux.
OK?

	* config/rs6000/rs6000.c (offsettable_ok_by_alignment): Ensure
	STRING_CST returns false.  Add assert.

Isn't this too conservative?  Shouldn't CONSTANT_ALIGNMENT increase
the alignment of STRING_CST to word-aligned?  The only problem is
structs?

- David

On Wed, Mar 2, 2011 at 2:34 AM, Alan Modra <amodra@gmail.com> wrote:
> Prior to a fix I committed recently on the gcc-4.5-ibm branch, we used
> to fail with a linker error: "relocation R_PPC64_TOC16_LO_DS not a
> multiple of 4" on code loading the "md_backup_data-1" string in the
> following testcase.
>
> typedef __signed__ char __s8;
> typedef unsigned char __u8;
> typedef __signed__ short __s16;
> typedef unsigned short __u16;
> typedef __signed__ int __s32;
> typedef unsigned int __u32;
> typedef __signed__ long __s64;
> typedef unsigned long __u64;
>
> static struct mdp_backup_super {
>  char magic[16];
>  __u8 set_uuid[16];
>  __u64 mtime;
>
>  __u64 devstart;
>  __u64 arraystart;
>  __u64 length;
>  __u32 sb_csum;
>  __u32 pad1;
>  __u64 devstart2;
>  __u64 arraystart2;
>  __u64 length2;
>  __u32 sb_csum2;
>  __u8 pad[512-68-32];
> } __attribute__((aligned(512))) bsb;
>
> void f (void)
> {
>  __builtin_memcpy (bsb.magic, "md_backup_data-1", 16);
> }
>
> The memcpy gets optimised to two ld insns followed by two std insns,
> with both the address of bsb and the string being toc-relative.
> bsb.magic is sufficiently aligned for the stores to be OK, but the
> string is not aligned at all;  The loads may well be at an address
> that is not a multiple of four.  It is also quite possible for the
> string to straddle a 32k boundary, resulting in rubbish being loaded
> in the second dword.
>
> On mainline, the situation is a little different due to changes in
> memref handling.  I seem to always get a VAR_DECL for the string,
> correctly reporting an alignment of one, so we don't use the invalid
> toc-relative address.  ie. I can't find a testcase for this potential
> problem on mainline.  What worries me is that this may simply be due
> to not trying enough compiler options.  Also, future changes in gcc
> might result in STRING_CST trees appearing as they do on 4.5.  So I'd
> like to apply the following patch.  Bootstrapped etc. powerpc64-linux.
> OK?
>
>        * config/rs6000/rs6000.c (offsettable_ok_by_alignment): Ensure
>        STRING_CST returns false.  Add assert.
>
> Index: gcc/config/rs6000/rs6000.c
> ===================================================================
> --- gcc/config/rs6000/rs6000.c  (revision 170373)
> +++ gcc/config/rs6000/rs6000.c  (working copy)
> @@ -7301,12 +7301,14 @@ offsettable_ok_by_alignment (tree decl)
>   if (!decl)
>     return true;
>
> -  if (TREE_CODE (decl) != VAR_DECL
> -      && TREE_CODE (decl) != PARM_DECL
> -      && TREE_CODE (decl) != RESULT_DECL
> -      && TREE_CODE (decl) != FIELD_DECL)
> +  if (TREE_CODE (decl) == FUNCTION_DECL)
>     return true;
>
> +  if (CONSTANT_CLASS_P (decl))
> +    return TREE_CODE (decl) != STRING_CST;
> +
> +  gcc_assert (DECL_P (decl));
> +
>   if (!DECL_SIZE_UNIT (decl))
>     return false;
>
>
> --
> Alan Modra
> Australia Development Lab, IBM
>

On Sun, Mar 06, 2011 at 07:01:44PM -0500, David Edelsohn wrote:
> Isn't this too conservative?  Shouldn't CONSTANT_ALIGNMENT increase
> the alignment of STRING_CST to word-aligned?

Yes and yes.  Thanks for the correction.  I'll update the patch.

> The only problem is structs?

No, I don't think there is anything special about the struct, just
that it was aligned more that usual.