Re: [PATCH 1/2] x86: don't suppress overflow diagnostics in x32 mode

["H.J. Lu" <hjl.tools@gmail.com>]

On Fri, Feb 25, 2022 at 3:19 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> Unlike in 64-bit mode, where values wrap at the 64-bit boundary anyway,
> there's no wrapping at the 32-bit boundary here, and hence overflow
> detection shouldn't be suppressed just because rela relocations are
> going to be used.
>
> The extra check against NO_RELOC is actually a result of an ilp32 test
> otherwise failing. But thinking about it, reporting overflows for
> not-really-relocations (typically because of earlier errors) makes
> little sense in general. Perhaps this should even be extended to non-
> 64-bit modes.
> ---
> After 862be3fb9a6d ("Disallow 64bit relocations in x32 mode") introduced
> the checking for 64-bit relocations in tc_gen_reloc(), d7921315bafb
> ("Check R_X86_64_32 overflow and allow R_X86_64_64 for x32") removed
> BFD_RELOC_64 from the set again. Hence besides the error messages
> triggered by the change here for 32-bit (and smaller) relocations, for
> that 64-bit one some diagnostic is needed as well. But that'll be the
> topic of another patch, as that's affecting at least RISC-V as well.
>
> Of course I have to admit that I don't really see why e.g.
> BFD_RELOC_64_PCREL cannot be represented (as the diagnostic puts it) -
> it's all a matter of whether the addend fits. The relocated field can
> hold all values.
>
> As to NO_RELOC: While looking around I didn't find uses of this value
> which aren't accompanied by as_bad() invocations. Yet I'm still somewhat
> uncertain whether the added check may not end up suppressing necessary
> overflow checks.
>
> Note that the as_bad() triggered here is actually unhelpful: It reports
> the already truncated value (which does fit in 32 bits), leaving it to
> the user to infer that the real value was actually larger.
>
> Note further that similar diagnostics ought to be emitted for 32-bit
> code, but they are hidden by the effects of a442cac5084e ("ix86: wrap
> constants"). That change effectively dealt with one aspect of
> inconsistencies between BFD64 and !BFD64 builds by introducing another
> one: Numbers wider than 32 bits ar represented as bignum in !BFD64
> builds, and bignums won't be accepted as relocation addends. Hence we
> have yet again a case where code would assemble in one case, but
> encounter errors in the other. It _may_ be acceptable in this case,
> where wider-than-32-bits values are supplied from the source file rather
> than being the result of calculations overflowing in 32 bits.
>
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -12790,7 +12790,8 @@ md_apply_fix (fixS *fixP, valueT *valP,
>  #endif
>    else if (use_rela_relocations)
>      {
> -      fixP->fx_no_overflow = 1;
> +      if (!disallow_64bit_reloc || fixP->fx_r_type == NO_RELOC)
> +       fixP->fx_no_overflow = 1;
>        /* Remember value for tc_gen_reloc.  */
>        fixP->fx_addnumber = value;
>        value = 0;
> --- a/gas/testsuite/gas/i386/ilp32/reloc64.l
> +++ b/gas/testsuite/gas/i386/ilp32/reloc64.l
> @@ -52,3 +52,16 @@
>  .*:176: Error: .*
>  .*:177: Error: .*
>  .*:189: Error: .*
> +.*:192: Error: .* too large for field of 4 bytes at .*
> +.*:193: Error: .* too large for field of 4 bytes at .*
> +.*:194: Error: .* too large for field of 4 bytes at .*
> +.*:195: Error: .* too large for field of 4 bytes at .*
> +.*:196: Error: .* too large for field of 2 bytes at .*
> +.*:196: Error: .* too large for field of 1 byte at .*
> +.*:197: Error: .* too large for field of 2 bytes at .*
> +.*:197: Error: .* too large for field of 1 byte at .*
> +.*:200: Error: .* too large for field of 4 bytes at .*
> +.*:201: Error: .* too large for field of 2 bytes at .*
> +.*:202: Error: .* too large for field of 2 bytes at .*
> +.*:203: Error: .* too large for field of 1 byte at .*
> +.*:204: Error: .* too large for field of 1 byte at .*
> --- a/gas/testsuite/gas/i386/ilp32/reloc64.s
> +++ b/gas/testsuite/gas/i386/ilp32/reloc64.s
> @@ -187,3 +187,18 @@ bad        .byte   xtrn@tpoff
>         .long   xtrn@got - 4
>         .long   xtrn@got + 4
>  bad    .long   xtrn@plt - .
> +
> +       .text
> +bad    add     $x+0x123456789, %rax
> +bad    add     $x+0x123456789, %eax
> +bad    add     x+0x123456789, %eax
> +bad    add     x+0x123456789(%eax), %eax
> +bad    enter   $x+0x123456789, $x+0x123456789
> +bad    enter   $x+0x12345, $x+0x123
> +
> +       .data
> +bad    .long x+0x123456789
> +bad    .word x+0x123456789
> +bad    .word x+0x12345
> +bad    .byte x+0x123456789
> +bad    .byte x+0x123
>

LGTM.

Thanks.

-- 
H.J.