Re: RFC: Should AArch64 *_NC relocs complain on overflow ?

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 08/02/16 12:39, Nick Clifton wrote:
> Hi Marcus, Hi Jiong,
> 
>> NC relocations do complain if the relocation requires some minimum alignment 
>> and the final relocated value does not meet that requirement.  Consider, for 
>> example, instructions with an implicit scaling of an address offset, 
>> relocations used for those instruction, including the NC form will gripe if 
>> the offset being inserted into the instruction is not a multiple of the 
>> implicit scaling implied.
> 
> OK that makes sense, 
> 
> It would be nice if we could generate a more helpful warning message for the user.
> What do you think of a patch like the one below ?  With this applied we get output 
> like the following:
> 
> 1.c:(.text+0x4): relocation truncated to fit: R_AARCH64_LDST32_ABS_LO12_NC against symbol `var_2' defined in COMMON section in tmpdir/dump1.o
> 1.c:(.text+0x4): warning: Possibly the symbol is being referenced via a differently aligned type to its declaration>, no expected output
> 
> Cheers
>   Nick
> 
> 
> aarch64-reloc-overflow.patch
> 
> 
> diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c
> index 292470df..be0ddb5 100644
> --- a/bfd/elfnn-aarch64.c
> +++ b/bfd/elfnn-aarch64.c
> @@ -6405,10 +6405,6 @@ elfNN_aarch64_relocate_section (bfd *output_bfd,
>  	  break;
>  	}
>  
> -      if (!save_addend)
> -	addend = 0;
> -
> -
>        /* Dynamic relocs are not propagated for SEC_DEBUGGING sections
>           because such sections are not SEC_ALLOC and thus ld.so will
>           not process them.  */
> @@ -6448,6 +6444,31 @@ elfNN_aarch64_relocate_section (bfd *output_bfd,
>  		     name, input_bfd, input_section, rel->r_offset);
>  		  return FALSE;
>  		}
> +	      /* Overflow can occur when a variable is referenced with a type
> +		 that has a larger alignment than the type with which it was
> +		 declared. eg:
> +		   file1.c: extern int foo; int a (void) { return foo; }
> +		   file2.c: char bar, foo, baz;
> +		 If the variable is placed into a data section at an offset
> +		 that is incompatible with the larger alignment requirement
> +		 overflow will occur.  (Strictly speaking this is not overflow
> +		 but rather an alignment problem, but the bfd_reloc_ error
> +		 enum does not have a value to cover that situation).
> +
> +		 Try to catch this situation here and provide a more helpful
> +		 error message to the user.  */
> +	      if (addend & ((1 << howto->rightshift) - 1)
> +		  /* FIXME: Are we testing all of the appropriate reloc
> +		     types here ?  */
> +		  && (real_r_type == BFD_RELOC_AARCH64_LDST16_LO12
> +		      || real_r_type == BFD_RELOC_AARCH64_LDST32_LO12
> +		      || real_r_type == BFD_RELOC_AARCH64_LDST64_LO12
> +		      || real_r_type == BFD_RELOC_AARCH64_LDST128_LO12))

Those are checking relocations (don't have _NC in the name), so I'd
expect that they already check alignment as part of their standard
overflow test (if they don't that's probably a different bug).

For the _NC versions, those would probably the most common, but there is
quite a long list of relocations that have alignment checks.  It would
be better if we could have an alignment check field in the standard
RELOC descriptor field that is distinct from the overflow check test.

> +		{
> +		  info->callbacks->warning
> +		    (info, _("Possibly the symbol is being referenced via a differently aligned type to its declaration"),
> +		     name, input_bfd, input_section, rel->r_offset);
> +		}
>  	      break;
>  
>  	    case bfd_reloc_undefined:
> @@ -6482,6 +6503,9 @@ elfNN_aarch64_relocate_section (bfd *output_bfd,
>  	      break;
>  	    }
>  	}
> +
> +      if (!save_addend)
> +	addend = 0;
>      }
>  
>    return TRUE;
> diff --git a/ld/testsuite/ld-aarch64/aarch64-elf.exp b/ld/testsuite/ld-aarch64/aarch64-elf.exp
> index 939539e..d0b33cf 100644
> --- a/ld/testsuite/ld-aarch64/aarch64-elf.exp
> +++ b/ld/testsuite/ld-aarch64/aarch64-elf.exp
> @@ -159,6 +159,8 @@ run_dump_test "emit-relocs-537"
>  run_dump_test "emit-relocs-537-overflow"
>  run_dump_test "emit-relocs-538"
>  
> +run_dump_test "reloc-overflow-bad"
> +
>  # test addend correctness when --emit-relocs specified for non-relocatable obj.
>  run_dump_test "emit-relocs-local-addend"
>  # test addend correctness when -r specified.
> --- /dev/null	2016-02-08 07:59:08.480962732 +0000
> +++ ld/testsuite/ld-aarch64/reloc-overflow-bad.d	2016-02-08 12:34:55.696124085 +0000
> @@ -0,0 +1,7 @@
> +#source: reloc-overflow-1.s
> +#source: reloc-overflow-2.s
> +#ld: -e0
> +#error: .*Possibly the symbol.*
> +
> +
> +
> --- /dev/null	2016-02-08 07:59:08.480962732 +0000
> +++ ld/testsuite/ld-aarch64/reloc-overflow-1.s	2016-02-08 12:30:59.584834873 +0000
> @@ -0,0 +1,14 @@
> +        .file   "1.c"
> +        .text
> +        .align  2
> +        .p2align 3,,7
> +        .global dec
> +        .arch armv8-a+fp+simd
> +        //.tune generic
> +        .type   dec, %function
> +dec:
> +        adrp    x0, var_2
> +        ldr     w0, [x0, #:lo12:var_2]
> +        ret
> +        .size   dec, .-dec
> +        .ident  "GCC: (GNU) 6.0.0 20160208 (experimental) [trunk revision 233206]"
> --- /dev/null	2016-02-08 07:59:08.480962732 +0000
> +++ ld/testsuite/ld-aarch64/reloc-overflow-2.s	2016-02-08 12:31:36.679037435 +0000
> @@ -0,0 +1,5 @@
> +        .file   "2.c"
> +        .comm   var_3,1,1
> +        .comm   var_2,1,1
> +        .comm   var_1,1,1
> +        .ident  "GCC: (GNU) 6.0.0 20160208 (experimental) [trunk revision 233206]"
>