public inbox for binutils@sourceware.org
 help / color / mirror / Atom feed
From: Jan Beulich <jbeulich@suse.com>
To: Binutils <binutils@sourceware.org>
Subject: [PATCH 1/2] x86: don't suppress overflow diagnostics in x32 mode
Date: Fri, 25 Feb 2022 12:19:42 +0100	[thread overview]
Message-ID: <9d8645a1-c97d-ef32-0f96-366f0ae43f06@suse.com> (raw)
In-Reply-To: <d69de13e-e169-8ac2-62a3-8b28b8fad0f3@suse.com>

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


  reply	other threads:[~2022-02-25 11:19 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-25 11:18 [PATCH 0/2] x86/x32: relocation overflow detection Jan Beulich
2022-02-25 11:19 ` Jan Beulich [this message]
2022-02-25 15:14   ` [PATCH 1/2] x86: don't suppress overflow diagnostics in x32 mode H.J. Lu
2022-02-25 11:20 ` [PATCH 2/2] ELF32: don't silently truncate relocation addends Jan Beulich
2022-02-27 14:01   ` Alan Modra
2022-02-28  7:21     ` Jan Beulich
2022-03-21 14:16 ` [PATCH v2] " Jan Beulich
2022-03-22 14:20   ` Nick Clifton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9d8645a1-c97d-ef32-0f96-366f0ae43f06@suse.com \
    --to=jbeulich@suse.com \
    --cc=binutils@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).