PATCH: Properly dump addend in readelf

PATCH: Properly dump addend in readelf

[H.J. Lu]

"readelf -r" failed to dump addend in 32bit ELF with 64bit VMA.  It
happpens on Linux/mips64:

FAIL: ld-mips-elf/reloc-1-n32

Also we can't assume long is always 64bit.  This patch uses long long
instead.  OK to install?

Thanks.


H.J.
----
2010-10-01  H.J. Lu  <hongjiu.lu@intel.com>

	* readelf.c (dump_relocations): Properly dump addend.

diff --git a/binutils/readelf.c b/binutils/readelf.c
index b91c5ba..876827d 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -1410,12 +1410,17 @@ dump_relocations (FILE * file,
 
 	      if (is_rela)
 		{
-		  long off = (long) (bfd_signed_vma) rels[i].r_addend;
+		  long long off;
+
+		  if (is_32bit_elf)
+		    off = (long long) (int) rels[i].r_addend;
+		  else
+		    off = rels[i].r_addend;
 
 		  if (off < 0)
-		    printf (" - %lx", - off);
+		    printf (" - %llx", - off);
 		  else
-		    printf (" + %lx", off);
+		    printf (" + %llx", off);
 		}
 	    }
 	}

Re: PATCH: Properly dump addend in readelf

[Joseph S. Myers]

On Fri, 1 Oct 2010, H.J. Lu wrote:

>  		  if (off < 0)
> -		    printf (" - %lx", - off);
> +		    printf (" - %llx", - off);
>  		  else
> -		    printf (" + %lx", off);
> +		    printf (" + %llx", off);

Use of %ll formats isn't portable across hosts; MinGW needs %I64.  See 
BFD_VMA_FMT in bfd-in.h, for example, or print_vma in binutils/prdbg.c.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: PATCH: Properly dump addend in readelf

[H.J. Lu]

On Sat, Oct 2, 2010 at 5:17 AM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Fri, 1 Oct 2010, H.J. Lu wrote:
>
>>                 if (off < 0)
>> -                 printf (" - %lx", - off);
>> +                 printf (" - %llx", - off);
>>                 else
>> -                 printf (" + %lx", off);
>> +                 printf (" + %llx", off);
>
> Use of %ll formats isn't portable across hosts; MinGW needs %I64.  See
> BFD_VMA_FMT in bfd-in.h, for example, or print_vma in binutils/prdbg.c.
>


BFD_VMA_FMT doesn't always work with long long. llx has been used:

dwarf.c:  snprintf (buff, sizeof (buff), "%16.16llx ", val);
nm.c:static char value_format_64bit[] = "%016llx";
prdbg.c:	sprintf (buf, "0x%llx", (unsigned long long) vma);
readelf.c:		  ? "%16.16llx  %16.16llx "
readelf.c:		  : "%12.12llx  %12.12llx ",
strings.c:	        printf ("%7llx ", (unsigned long long) start);

print_vma will fail many tests due to leading 0s.


-- 
H.J.

Re: PATCH: Properly dump addend in readelf

[Joseph S. Myers]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3657 bytes --]

On Sat, 2 Oct 2010, H.J. Lu wrote:

> On Sat, Oct 2, 2010 at 5:17 AM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> > On Fri, 1 Oct 2010, H.J. Lu wrote:
> >
> >>                 if (off < 0)
> >> -                 printf (" - %lx", - off);
> >> +                 printf (" - %llx", - off);
> >>                 else
> >> -                 printf (" + %lx", off);
> >> +                 printf (" + %llx", off);
> >
> > Use of %ll formats isn't portable across hosts; MinGW needs %I64.  See
> > BFD_VMA_FMT in bfd-in.h, for example, or print_vma in binutils/prdbg.c.
> >
> 
> 
> BFD_VMA_FMT doesn't always work with long long. llx has been used:

I was giving examples of how this can be done portably.

> dwarf.c:  snprintf (buff, sizeof (buff), "%16.16llx ", val);

This has a proper __MSVCRT__ conditional and was one of the examples I 
gave above.

> nm.c:static char value_format_64bit[] = "%016llx";

This is the only case where there actually appears to be a bug in the 
existing code.  OK to commit the patch below (untested) to fix it?

> prdbg.c:	sprintf (buf, "0x%llx", (unsigned long long) vma);

This has __MSVCRT__ conditionals.

> readelf.c:		  ? "%16.16llx  %16.16llx "
> readelf.c:		  : "%12.12llx  %12.12llx ",

This has __MSVCRT__ conditionals.

> strings.c:	        printf ("%7llx ", (unsigned long long) start);

This has __MSVCRT__ conditionals.

> print_vma will fail many tests due to leading 0s.

I was giving examples of how this should be implemented to work across 
hosts rather than saying that an existing function could be used without 
changes.

I'm aware that gold has several potentially problematic "ll" formats - 
it's quite possible that such diagnostics are not covered by the gold 
testsuite, and use of gold as a cross-linker on Windows hosts may have 
been limited.  (Also, whether a particular printf-family function ends up 
getting replaced by a MinGW version that supports "ll", or goes through to 
an MSVCRT version, may depend on the details of the MinGW version and 
which libraries you link with - MinGW supports various different versions 
of the Windows DLLs.)  But with my patch applied I don't see any in the 
binutils/ directory that are apparent from a grep.  In bfd/ there are some 
in coff-rs6000.c, and coffcode.h conditional on XCOFF64.  I don't see any 
problem uses in gas (without a proper conditional) or ld.

2010-10-02  Joseph Myers  <joseph@codesourcery.com>

	* nm.c (value_format_64bit): Define appropriately for __MSVCRT__.
	(set_print_radix): Update for __MSVCRT__ definition of
	value_format_64bit.

Index: nm.c
===================================================================
RCS file: /cvs/src/src/binutils/nm.c,v
retrieving revision 1.63
diff -u -p -r1.63 nm.c
--- nm.c	6 Jan 2010 08:48:19 -0000	1.63
+++ nm.c	2 Oct 2010 17:02:14 -0000
@@ -164,7 +164,11 @@ static char value_format_32bit[] = "%08l
 #if BFD_HOST_64BIT_LONG
 static char value_format_64bit[] = "%016lx";
 #elif BFD_HOST_64BIT_LONG_LONG
+#ifndef __MSVCRT__
 static char value_format_64bit[] = "%016llx";
+#else
+static char value_format_64bit[] = "%016I64x";
+#endif
 #endif
 static int print_width = 0;
 static int print_radix = 16;
@@ -284,7 +288,11 @@ set_print_radix (char *radix)
 #if BFD_HOST_64BIT_LONG
       value_format_64bit[5] = *radix;
 #elif BFD_HOST_64BIT_LONG_LONG
+#ifndef __MSVCRT__
       value_format_64bit[6] = *radix;
+#else
+      value_format_64bit[7] = *radix;
+#endif
 #endif
       other_format[3] = desc_format[3] = *radix;
       break;

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: PATCH: Properly dump addend in readelf

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

On Sat, Oct 2, 2010 at 10:22 AM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Sat, 2 Oct 2010, H.J. Lu wrote:
>
>> On Sat, Oct 2, 2010 at 5:17 AM, Joseph S. Myers <joseph@codesourcery.com> wrote:
>> > On Fri, 1 Oct 2010, H.J. Lu wrote:
>> >
>> >>                 if (off < 0)
>> >> -                 printf (" - %lx", - off);
>> >> +                 printf (" - %llx", - off);
>> >>                 else
>> >> -                 printf (" + %lx", off);
>> >> +                 printf (" + %llx", off);
>> >
>> > Use of %ll formats isn't portable across hosts; MinGW needs %I64.  See
>> > BFD_VMA_FMT in bfd-in.h, for example, or print_vma in binutils/prdbg.c.
>> >
>>
>>
>> BFD_VMA_FMT doesn't always work with long long. llx has been used:
>
> I was giving examples of how this can be done portably.
>

Here is a new patch. It also fixed potential problem with 32bit ELF
and 64bit VMA:

	      aux->table[i].start.offset  = rp->r_addend + sym->st_value;
	      aux->table[i].end.offset    = rp->r_addend + sym->st_value;
	      aux->table[i].info.offset   = rp->r_addend + sym->st_value;
	      aux->table[i].start.offset  = sym->st_value + rp->r_addend;
	      aux->table[i].end.offset    = sym->st_value + rp->r_addend;
	    addend += rp->r_addend;

OK to install?

Thanks.


-- 
H.J.
----
2010-10-02  H.J. Lu  <hongjiu.lu@intel.com>

	* readelf.c (slurp_rela_relocs): Properly convert r_addend.
	(dump_relocations): Properly dump r_addend.

[-- Attachment #2: binutils-addend-2.patch --]
[-- Type: text/plain, Size: 1004 bytes --]

2010-10-02  H.J. Lu  <hongjiu.lu@intel.com>

	* readelf.c (slurp_rela_relocs): Properly convert r_addend.
	(dump_relocations): Properly dump r_addend.

diff --git a/binutils/readelf.c b/binutils/readelf.c
index b91c5ba..12d0314 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -734,7 +734,8 @@ slurp_rela_relocs (FILE * file,
 	{
 	  relas[i].r_offset = BYTE_GET (erelas[i].r_offset);
 	  relas[i].r_info   = BYTE_GET (erelas[i].r_info);
-	  relas[i].r_addend = BYTE_GET (erelas[i].r_addend);
+	  relas[i].r_addend
+	    = (bfd_signed_vma) (int) BYTE_GET (erelas[i].r_addend);
 	}
 
       free (erelas);
@@ -1410,12 +1411,12 @@ dump_relocations (FILE * file,
 
 	      if (is_rela)
 		{
-		  long off = (long) (bfd_signed_vma) rels[i].r_addend;
+		  bfd_signed_vma off = rels[i].r_addend;
 
 		  if (off < 0)
-		    printf (" - %lx", - off);
+		    printf (" - %" BFD_VMA_FMT "x", - off);
 		  else
-		    printf (" + %lx", off);
+		    printf (" + %" BFD_VMA_FMT "x", off);
 		}
 	    }
 	}

Re: PATCH: Properly dump addend in readelf

[Maciej W. Rozycki]

On Sat, 2 Oct 2010, Joseph S. Myers wrote:

> Index: nm.c
> ===================================================================
> RCS file: /cvs/src/src/binutils/nm.c,v
> retrieving revision 1.63
> diff -u -p -r1.63 nm.c
> --- nm.c	6 Jan 2010 08:48:19 -0000	1.63
> +++ nm.c	2 Oct 2010 17:02:14 -0000
> @@ -164,7 +164,11 @@ static char value_format_32bit[] = "%08l
>  #if BFD_HOST_64BIT_LONG
>  static char value_format_64bit[] = "%016lx";
>  #elif BFD_HOST_64BIT_LONG_LONG
> +#ifndef __MSVCRT__
>  static char value_format_64bit[] = "%016llx";
> +#else
> +static char value_format_64bit[] = "%016I64x";
> +#endif
>  #endif
>  static int print_width = 0;
>  static int print_radix = 16;
> @@ -284,7 +288,11 @@ set_print_radix (char *radix)
>  #if BFD_HOST_64BIT_LONG
>        value_format_64bit[5] = *radix;
>  #elif BFD_HOST_64BIT_LONG_LONG
> +#ifndef __MSVCRT__
>        value_format_64bit[6] = *radix;
> +#else
> +      value_format_64bit[7] = *radix;
> +#endif
>  #endif
>        other_format[3] = desc_format[3] = *radix;
>        break;

 Hmm, how about using:

	value_format_64bit[sizeof (value_format_64bit) - 2] = *radix;

and getting rid of the fragile #ifdef mess?  Likewise the variable might 
only be defined once, e.g.:

#if BFD_HOST_64BIT_LONG
# define NM_64BIT_FMT "%016lx"
#elif BFD_HOST_64BIT_LONG_LONG && !defined (__MSVCRT__)
# define NM_64BIT_FMT "%016llx"
#else
# define NM_64BIT_FMT "%016I64x"
#endif
static char value_format_64bit[] = NM_64BIT_FMT;

It would seem cleaner to me this way.

  Maciej

Re: PATCH: Properly dump addend in readelf

[Alan Modra]

On Sat, Oct 02, 2010 at 03:27:17PM -0700, H.J. Lu wrote:
> diff --git a/binutils/readelf.c b/binutils/readelf.c
> index b91c5ba..12d0314 100644
> --- a/binutils/readelf.c
> +++ b/binutils/readelf.c
> @@ -734,7 +734,8 @@ slurp_rela_relocs (FILE * file,
>  	{
>  	  relas[i].r_offset = BYTE_GET (erelas[i].r_offset);
>  	  relas[i].r_info   = BYTE_GET (erelas[i].r_info);
> -	  relas[i].r_addend = BYTE_GET (erelas[i].r_addend);
> +	  relas[i].r_addend
> +	    = (bfd_signed_vma) (int) BYTE_GET (erelas[i].r_addend);
>  	}
>  
>        free (erelas);

byte_get_signed is better here.  Patch otherwise OK.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: Properly dump addend in readelf

[Alan Modra]

On Sat, Oct 02, 2010 at 05:22:39PM +0000, Joseph S. Myers wrote:
> 	* nm.c (value_format_64bit): Define appropriately for __MSVCRT__.
> 	(set_print_radix): Update for __MSVCRT__ definition of
> 	value_format_64bit.

OK, thanks!

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: Properly dump addend in readelf

[Alan Modra]

On Mon, Oct 04, 2010 at 12:03:53AM +0100, Maciej W. Rozycki wrote:
>  Hmm, how about using:
> 
> 	value_format_64bit[sizeof (value_format_64bit) - 2] = *radix;

Yes, that's nicer.  Patch preapproved.

-- 
Alan Modra
Australia Development Lab, IBM

Re: PATCH: Properly dump addend in readelf

[Joseph S. Myers]

On Fri, 8 Oct 2010, Alan Modra wrote:

> On Sat, Oct 02, 2010 at 05:22:39PM +0000, Joseph S. Myers wrote:
> > 	* nm.c (value_format_64bit): Define appropriately for __MSVCRT__.
> > 	(set_print_radix): Update for __MSVCRT__ definition of
> > 	value_format_64bit.
> 
> OK, thanks!

I've now committed this patch - I'll leave it to Maciej to apply his 
followup cleanup to this code.

-- 
Joseph S. Myers
joseph@codesourcery.com