[PATCH] Fix gdb/coffread.c build on 32bit architectures

[PATCH] Fix gdb/coffread.c build on 32bit architectures

[Mark Wielaard]

The getsymname function tries to emit an error using %ld for an
uintptr_t argument. Use PRIxPTR instead. Which works on any architecture
for uintptr_t.
---
 gdb/coffread.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index ae7632d49cb..c609c963453 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -1325,7 +1325,7 @@ getsymname (struct internal_syment *symbol_entry)
   if (symbol_entry->_n._n_n._n_zeroes == 0)
     {
       if (symbol_entry->_n._n_n._n_offset > stringtab_length)
-	error (_("COFF Error: string table offset (%ld) outside string table (length %ld)"),
+	error (_("COFF Error: string table offset (%" PRIxPTR ") outside string table (length %ld)"),
 	       symbol_entry->_n._n_n._n_offset, stringtab_length);
       result = stringtab + symbol_entry->_n._n_n._n_offset;
     }
-- 
2.39.3

Re: [PATCH] Fix gdb/coffread.c build on 32bit architectures

[Keith Seitz]

On 8/25/23 14:16, Mark Wielaard wrote:
> The getsymname function tries to emit an error using %ld for an
> uintptr_t argument. Use PRIxPTR instead. Which works on any architecture
> for uintptr_t.

Doh! Thank you for catching that!

I should think this qualifies under the "obvious" rule...

Keith

Re: [PATCH] Fix gdb/coffread.c build on 32bit architectures

[Tom Tromey]

>>>>> "Mark" == Mark Wielaard <mark@klomp.org> writes:

Hi Mark.  Thanks for the patch.

Mark> +	error (_("COFF Error: string table offset (%" PRIxPTR ") outside string table (length %ld)"),

gdb doesn't use these PRI macros.  Instead you'd write something like:

    error ("...%s...", hex_string (value))

Tom

Re: [PATCH] Fix gdb/coffread.c build on 32bit architectures

[Keith Seitz]

On 8/28/23 07:08, Tom Tromey wrote:
>>>>>> "Mark" == Mark Wielaard <mark@klomp.org> writes:
> 
> Hi Mark.  Thanks for the patch.
> 
> Mark> +	error (_("COFF Error: string table offset (%" PRIxPTR ") outside string table (length %ld)"),
> 
> gdb doesn't use these PRI macros.  Instead you'd write something like:
> 
>      error ("...%s...", hex_string (value))

Yeah, I remembered this over the weekend. I also think this isn't exactly the
nicest construct for i18n.

May I assume that converting to hex_string for printing these offsets is obvious
or pre-approved?

Keith

Re: [PATCH] Fix gdb/coffread.c build on 32bit architectures

[Keith Seitz]

On 8/28/23 07:20, Keith Seitz via Gdb-patches wrote:
> 
> May I assume that converting to hex_string for printing these offsets is obvious
> or pre-approved?

Or to be more explicit (pardon me -- ECAFFEINE):

Use hex_string to output hexidecimal values

Late last week, I committed a patch that used %ld to output the
value of some offsets in coffread.c.  This caused a build failure
on 32-bit hardware, and Mark Wielaard offered up a quick fix by
using PRIxPTR.

As mentioned by Tom Tromey, the correct way to handle this is by using
a utility function such as hex_string.  This patch converts the two
%ld formats into %s w/hex_string instead.
---
  gdb/coffread.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index c609c963453..3caa7eb3175 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -1325,8 +1325,8 @@ getsymname (struct internal_syment *symbol_entry)
    if (symbol_entry->_n._n_n._n_zeroes == 0)
      {
        if (symbol_entry->_n._n_n._n_offset > stringtab_length)
-	error (_("COFF Error: string table offset (%" PRIxPTR ") outside string table (length %ld)"),
-	       symbol_entry->_n._n_n._n_offset, stringtab_length);
+	error (_("COFF Error: string table offset (%s) outside string table (length %s)"),
+	       hex_string (symbol_entry->_n._n_n._n_offset), hex_string (stringtab_length));
        result = stringtab + symbol_entry->_n._n_n._n_offset;
      }
    else
-- 
2.41.0

Re: [PATCH] Fix gdb/coffread.c build on 32bit architectures

[Mark Wielaard]

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

On Mon, 2023-08-28 at 08:08 -0600, Tom Tromey wrote:
> > > > > > "Mark" == Mark Wielaard <mark@klomp.org> writes:
> 
> Hi Mark.  Thanks for the patch.
> 
> Mark> +	error (_("COFF Error: string table offset (%" PRIxPTR ") outside string table (length %ld)"),
> 
> gdb doesn't use these PRI macros.

Not always, but there are various uses in the code base.
e.g. gdb/dwarf2/cooked-index.c uses
gdb_printf ("    DIE offset: 0x%" PRIx64 "\n", ...
As do gdb/bt-utils.c, gdb/btrace.c, gdb/netbsd-nat.c.
And bfd, opcodes, sim, etc. do use it.

>   Instead you'd write something like:
> 
>     error ("...%s...", hex_string (value))

OK, the attached seems to work on both 32 and 64 bit systems.

Cheers,

Mark

[-- Attachment #2: 0001-Use-hex_string-in-gdb-coffread.c-instead-of-PRIxPTR.patch --]
[-- Type: text/x-patch, Size: 1123 bytes --]

From 3aaddda2c6e3679579c455e7a71981ffe1724206 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Mon, 28 Aug 2023 16:30:14 +0200
Subject: [PATCH] Use hex_string in gdb/coffread.c instead of PRIxPTR

The getsymname function uses PRIxPTR to print and uintptr_t value in
an error message. Use hex_string instead.
---
 gdb/coffread.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/coffread.c b/gdb/coffread.c
index c609c963453..c2fe9fa1761 100644
--- a/gdb/coffread.c
+++ b/gdb/coffread.c
@@ -1325,8 +1325,8 @@ getsymname (struct internal_syment *symbol_entry)
   if (symbol_entry->_n._n_n._n_zeroes == 0)
     {
       if (symbol_entry->_n._n_n._n_offset > stringtab_length)
-	error (_("COFF Error: string table offset (%" PRIxPTR ") outside string table (length %ld)"),
-	       symbol_entry->_n._n_n._n_offset, stringtab_length);
+	error (_("COFF Error: string table offset (%s) outside string table (length %ld)"),
+	       hex_string (symbol_entry->_n._n_n._n_offset), stringtab_length);
       result = stringtab + symbol_entry->_n._n_n._n_offset;
     }
   else
-- 
2.41.0

Re: [PATCH] Fix gdb/coffread.c build on 32bit architectures

[Andreas Schwab]

On Aug 28 2023, Mark Wielaard wrote:

> e.g. gdb/dwarf2/cooked-index.c uses
> gdb_printf ("    DIE offset: 0x%" PRIx64 "\n", ...

There's actually sect_offset_str for this.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Fix gdb/coffread.c build on 32bit architectures

[Tom Tromey]

>>>>> "Mark" == Mark Wielaard <mark@klomp.org> writes:

Mark> OK, the attached seems to work on both 32 and 64 bit systems.

Thanks.

Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH] Fix gdb/coffread.c build on 32bit architectures

[Tom Tromey]

>>>>> "Mark" == Mark Wielaard <mark@klomp.org> writes:

Mark> +	error (_("COFF Error: string table offset (%" PRIxPTR ") outside string table (length %ld)"),

>> gdb doesn't use these PRI macros.

Mark> Not always, but there are various uses in the code base.

Yeah.  In gdb it's typical for things to creep in, but the existence of
something doesn't always make it ok.

Tom