bfd_[fs]printf_vma() truncating values

Message ID a10e1c34-ae22-dbb0-4d3a-f5941eceab7b@suse.com
State New
Headers show
Series
  • bfd_[fs]printf_vma() truncating values
Related show

Commit Message

Claudiu Zissulescu via Binutils May 3, 2021, 2:11 p.m.
When putting together 9a8041fd94b7 ("gas: drop sprint_value()") I failed
to pay attention to bfd_sprintf_vma() silently truncating values for
32-bit binaries. This isn't helpful in particular in diagnostics. I came
up with the change below, but at least arm, cris, mips, riscv, and rx
utilize the truncation (i.e. there are now testsuite failures in cases
where negative values get printed as huge positive ones, which now get 8
more 'f' prefixed).

One alternative might be to avoid using the functions for diagnostic
output (where out-of-range values are relevant to report), and instead
have callers use BFD_VMA_FMT (or [fs]printf_vma()) directly. However,
the limiting to 8 characters by the functions here (for small enough
values) seems desirable to me for consumption by humans.

As a result, I'd like to seek input towards possible other options first.

Thanks, Jan

Comments

Claudiu Zissulescu via Binutils May 4, 2021, 1:16 a.m. | #1
On Mon, May 03, 2021 at 04:11:11PM +0200, Jan Beulich via Binutils wrote:
> When putting together 9a8041fd94b7 ("gas: drop sprint_value()") I failed

> to pay attention to bfd_sprintf_vma() silently truncating values for

> 32-bit binaries.


Which is exactly what happens when building for a 32-bit target on a
32-bit host.  I don't see there is any problem here.

-- 
Alan Modra
Australia Development Lab, IBM
Claudiu Zissulescu via Binutils May 4, 2021, 6:22 a.m. | #2
On 04.05.2021 03:16, Alan Modra wrote:
> On Mon, May 03, 2021 at 04:11:11PM +0200, Jan Beulich via Binutils wrote:

>> When putting together 9a8041fd94b7 ("gas: drop sprint_value()") I failed

>> to pay attention to bfd_sprintf_vma() silently truncating values for

>> 32-bit binaries.

> 

> Which is exactly what happens when building for a 32-bit target on a

> 32-bit host.  I don't see there is any problem here.


Good point (I assume you additionally mean a !BFD64 build). But then
I'm observing (for i686) diagnostics like

Warning: 00000001000000f4 shortened to 000000f4
Error: value of 0000000100000090 too large for field of 4 bytes at 00000010

which now have become

Warning: 000000f4 shortened to 000000f4
Error: value of 00000090 too large for field of 4 bytes at 00000010

I suppose these diagnostics, which presumably wouldn't be there on a
!BFD64 build, are all bogus in the first place (ought to be consistent
between BFD64 and !BFD64 builds), but clearly one form at least makes
clear what is happening, while the other leaves one completely in the
dark as to what the assembler is really complaining about.

Jan

	.text

# Bogus "Warning: 00000001000000.. shortened to 00000000000000.." instances with a BFD64 assembler (and --32).
# Bogus "Error: value of 00000001000000.. too large for field of 4 bytes at ..." with a BFD64 assembler (and --32).

# One part of the solution here may be to truncate constants in i386_finalize_immediate() instead of sign-extending
# them.

# It is also somewhat surprising that AT&T and Intel modes behave the same here, despite the lack of a suffix
# making e.g. optimize_imm() behave differently. At the very least this hints at some code being unnecessary
# somewhere.

long:
	mov	$500 - 0x100, %eax
	mov	$500 + 0xffffff00, %edx
	mov	$val - 0x100, %eax
	mov	$val + 0xffffff00, %edx
	mov	$sym - 0x100, %eax
	mov	$sym + 0xffffff00, %edx
	mov	$sym + 500 - 0x100, %eax
	mov	$sym + 500 + 0xffffff00, %edx

	movl	$500 - 0x100, (%eax)
	movl	$500 + 0xffffff00, (%edx)
	movl	$val - 0x100, (%eax)
	movl	$val + 0xffffff00, (%edx)
	movl	$sym - 0x100, (%eax)
	movl	$sym + 0xffffff00, (%edx)
	movl	$sym + 500 - 0x100, (%eax)
	movl	$sym + 500 + 0xffffff00, (%edx)

	add	$500 - 0x100, %ecx
	add	$500 + 0xffffff00, %edx
	add	$val - 0x100, %ecx
	add	$val + 0xffffff00, %edx
	add	$sym - 0x100, %ecx
	add	$sym + 0xffffff00, %edx
	add	$sym + 500 - 0x100, %ecx
	add	$sym + 500 + 0xffffff00, %edx

	addl	$500 - 0x100, (%eax)
	addl	$500 + 0xffffff00, (%edx)
	addl	$val - 0x100, (%eax)
	addl	$val + 0xffffff00, (%edx)
	addl	$sym - 0x100, (%eax)
	addl	$sym + 0xffffff00, (%edx)
	addl	$sym + 500 - 0x100, (%eax)
	addl	$sym + 500 + 0xffffff00, (%edx)

	.intel_syntax noprefix

	mov	eax, 500 - 0x100
	mov	edx, 500 + 0xffffff00
	mov	eax, offset val - 0x100
	mov	edx, offset val + 0xffffff00
	mov	eax, offset sym - 0x100
	mov	edx, offset sym + 0xffffff00
	mov	eax, offset sym + 500 - 0x100
	mov	edx, offset sym + 500 + 0xffffff00

	mov	dword ptr [eax], 500 - 0x100
	mov	dword ptr [edx], 500 + 0xffffff00
	mov	dword ptr [eax], offset val - 0x100
	mov	dword ptr [edx], offset val + 0xffffff00
	mov	dword ptr [eax], offset sym - 0x100
	mov	dword ptr [edx], offset sym + 0xffffff00
	mov	dword ptr [eax], offset sym + 500 - 0x100
	mov	dword ptr [edx], offset sym + 500 + 0xffffff00

	add	ecx, 500 - 0x100
	add	edx, 500 + 0xffffff00
	add	ecx, offset val - 0x100
	add	edx, offset val + 0xffffff00
	add	ecx, offset sym - 0x100
	add	edx, offset sym + 0xffffff00
	add	ecx, offset sym + 500 - 0x100
	add	edx, offset sym + 500 + 0xffffff00

	add	dword ptr [eax], 500 - 0x100
	add	dword ptr [edx], 500 + 0xffffff00
	add	dword ptr [eax], offset val - 0x100
	add	dword ptr [edx], offset val + 0xffffff00
	add	dword ptr [eax], offset sym - 0x100
	add	dword ptr [edx], offset sym + 0xffffff00
	add	dword ptr [eax], offset sym + 500 - 0x100
	add	dword ptr [edx], offset sym + 500 + 0xffffff00

	.equ	val, 400

	ret
Claudiu Zissulescu via Binutils May 4, 2021, 11:06 a.m. | #3
On Tue, May 04, 2021 at 08:22:47AM +0200, Jan Beulich wrote:
> I suppose these diagnostics, which presumably wouldn't be there on a

> !BFD64 build, are all bogus in the first place (ought to be consistent

> between BFD64 and !BFD64 builds),


Yes, please fix those bugs.

-- 
Alan Modra
Australia Development Lab, IBM

Patch

--- a/bfd/bfd.c
+++ b/bfd/bfd.c
@@ -2208,9 +2208,9 @@  void
 bfd_sprintf_vma (bfd *abfd ATTRIBUTE_UNUSED, char *buf, bfd_vma value)
 {
 #ifdef BFD64
-  if (is32bit (abfd))
+  if (is32bit (abfd) && value == (value & 0xffffffff))
     {
-      sprintf (buf, "%08lx", (unsigned long) value & 0xffffffff);
+      sprintf (buf, "%08lx", (unsigned long) value);
       return;
     }
 #endif
@@ -2221,9 +2221,9 @@  void
 bfd_fprintf_vma (bfd *abfd ATTRIBUTE_UNUSED, void *stream, bfd_vma value)
 {
 #ifdef BFD64
-  if (is32bit (abfd))
+  if (is32bit (abfd) && value == (value & 0xffffffff))
     {
-      fprintf ((FILE *) stream, "%08lx", (unsigned long) value & 0xffffffff);
+      fprintf ((FILE *) stream, "%08lx", (unsigned long) value);
       return;
     }
 #endif
--- a/binutils/objdump.c
+++ b/binutils/objdump.c
@@ -4497,7 +4497,7 @@  dump_reloc_set (bfd *abfd, asection *sec
       {
 	char buf[30];
 
-	bfd_sprintf_vma (abfd, buf, (bfd_vma) -1);
+	bfd_sprintf_vma (abfd, buf, 0);
 	width = strlen (buf) - 7;
       }
     printf ("OFFSET %*s TYPE %*s VALUE \n", width, "", 12, "");