* [PATCH] nm: Enforce 32-bit width limit when printing 32-bit values
@ 2023-12-04 12:03 Nicolas Schier
2023-12-04 22:23 ` Alan Modra
0 siblings, 1 reply; 3+ messages in thread
From: Nicolas Schier @ 2023-12-04 12:03 UTC (permalink / raw)
To: binutils; +Cc: Nicolas Schier, Johannes Nixdorf
Enforce printing only lower-half of 64-bit values if 32-bit print width
is requested.
Printing 32-bit values >= 0x80000000 on MIPS32 causes effective output
of a sign-extended 64-bit value since commit 0e3c1eebb22 ("Remove use of
bfd_uint64_t and similar", 2022-05-27). While this might be considered
appropriate for a "signed address-space", the output differs from other
tools from binutils, e.g. objdump, which always prints 32-bit values as
32-bit values.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31096
Fixes: 0e3c1eebb22e ("Remove use of bfd_uint64_t and similar")
Signed-off-by: Nicolas Schier <n.schier@avm.de>
---
binutils/nm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/binutils/nm.c b/binutils/nm.c
index e4c8036df1b..7e42ce8f469 100644
--- a/binutils/nm.c
+++ b/binutils/nm.c
@@ -1821,6 +1821,9 @@ print_value (bfd *abfd ATTRIBUTE_UNUSED, bfd_vma val)
switch (print_width)
{
case 32:
+ printf (print_format_string, 0xffffffff & (uint64_t) val);
+ break;
+
case 64:
printf (print_format_string, (uint64_t) val);
break;
--
2.40.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] nm: Enforce 32-bit width limit when printing 32-bit values
2023-12-04 12:03 [PATCH] nm: Enforce 32-bit width limit when printing 32-bit values Nicolas Schier
@ 2023-12-04 22:23 ` Alan Modra
2023-12-05 10:28 ` Nicolas Schier
0 siblings, 1 reply; 3+ messages in thread
From: Alan Modra @ 2023-12-04 22:23 UTC (permalink / raw)
To: Nicolas Schier; +Cc: binutils, Johannes Nixdorf
On Mon, Dec 04, 2023 at 01:03:29PM +0100, Nicolas Schier wrote:
> Enforce printing only lower-half of 64-bit values if 32-bit print width
> is requested.
>
> Printing 32-bit values >= 0x80000000 on MIPS32 causes effective output
> of a sign-extended 64-bit value since commit 0e3c1eebb22 ("Remove use of
> bfd_uint64_t and similar", 2022-05-27). While this might be considered
Hmm, that's only partly true. Prior to commit 0e3c1eebb22 nm output
depended on the host unsigned long when printing "negative" symbol
values for 32-bit targets. Commit 0e3c1eebb22 made the output match
that seen with a 64-bit host unsigned long. The fact that nm output
changed depending on host is of course a bug.
> appropriate for a "signed address-space", the output differs from other
> tools from binutils, e.g. objdump, which always prints 32-bit values as
> 32-bit values.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31096
> Fixes: 0e3c1eebb22e ("Remove use of bfd_uint64_t and similar")
> Signed-off-by: Nicolas Schier <n.schier@avm.de>
> ---
> binutils/nm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/binutils/nm.c b/binutils/nm.c
> index e4c8036df1b..7e42ce8f469 100644
> --- a/binutils/nm.c
> +++ b/binutils/nm.c
> @@ -1821,6 +1821,9 @@ print_value (bfd *abfd ATTRIBUTE_UNUSED, bfd_vma val)
> switch (print_width)
> {
> case 32:
> + printf (print_format_string, 0xffffffff & (uint64_t) val);
> + break;
> +
This isn't correct. At least, it changes behaviour from that prior to
0e3c1eebb22 for nm -td output.
> case 64:
> printf (print_format_string, (uint64_t) val);
> break;
> --
> 2.40.1
If we want to make nm output for 32-bit targets match that seen on
32-bit hosts prior to commit 0e3c1eebb22, then something like the
following (only lightly tested) patch is needed. Please test.
diff --git a/binutils/nm.c b/binutils/nm.c
index 54ee8182b2e..a811defba82 100644
--- a/binutils/nm.c
+++ b/binutils/nm.c
@@ -169,7 +169,7 @@ static const struct output_fns formats[FORMAT_MAX] =
/* The output format to use. */
static const struct output_fns *format = &formats[FORMAT_DEFAULT];
static unsigned int print_format = FORMAT_DEFAULT;
-static const char *print_format_string = NULL;
+static char print_format_string[10];
/* Command options. */
@@ -1512,37 +1512,8 @@ display_rel_file (bfd *abfd, bfd *archive_bfd)
/* Construct a formatting string for printing symbol values. */
-static const char *
-get_print_format (void)
-{
- const char * padding;
- if (print_format == FORMAT_POSIX || print_format == FORMAT_JUST_SYMBOLS)
- {
- /* POSIX compatible output does not have any padding. */
- padding = "";
- }
- else if (print_width == 32)
- {
- padding ="08";
- }
- else /* print_width == 64 */
- {
- padding = "016";
- }
-
- const char * radix = NULL;
- switch (print_radix)
- {
- case 8: radix = PRIo64; break;
- case 10: radix = PRId64; break;
- case 16: radix = PRIx64; break;
- }
-
- return concat ("%", padding, radix, NULL);
-}
-
static void
-set_print_width (bfd *file)
+set_print_format (bfd *file)
{
print_width = bfd_get_arch_size (file);
@@ -1559,8 +1530,43 @@ set_print_width (bfd *file)
else
print_width = 32;
}
- free ((char *) print_format_string);
- print_format_string = get_print_format ();
+
+ char *p = print_format_string;
+ *p++ = '%';
+ if (print_format == FORMAT_POSIX || print_format == FORMAT_JUST_SYMBOLS)
+ {
+ /* POSIX compatible output does not have any padding. */
+ }
+ else if (print_width == 32)
+ {
+ *p++ = '0';
+ *p++ = '8';
+ }
+ else /* print_width == 64 */
+ {
+ *p++ = '0';
+ *p++ = '1';
+ *p++ = '6';
+ }
+
+ if (print_width == 32)
+ {
+ switch (print_radix)
+ {
+ case 8: strcpy (p, PRIo32); break;
+ case 10: strcpy (p, PRId32); break;
+ case 16: strcpy (p, PRIx32); break;
+ }
+ }
+ else
+ {
+ switch (print_radix)
+ {
+ case 8: strcpy (p, PRIo64); break;
+ case 10: strcpy (p, PRId64); break;
+ case 16: strcpy (p, PRIx64); break;
+ }
+ }
}
static void
@@ -1588,7 +1594,7 @@ display_archive (bfd *file)
if (bfd_check_format_matches (arfile, bfd_object, &matching))
{
- set_print_width (arfile);
+ set_print_format (arfile);
format->print_archive_member (bfd_get_filename (file),
bfd_get_filename (arfile));
display_rel_file (arfile, file);
@@ -1644,7 +1650,7 @@ display_file (char *filename)
}
else if (bfd_check_format_matches (file, bfd_object, &matching))
{
- set_print_width (file);
+ set_print_format (file);
format->print_object_filename (filename);
display_rel_file (file, NULL);
}
@@ -1821,6 +1827,9 @@ print_value (bfd *abfd ATTRIBUTE_UNUSED, bfd_vma val)
switch (print_width)
{
case 32:
+ printf (print_format_string, (uint32_t) val);
+ break;
+
case 64:
printf (print_format_string, (uint64_t) val);
break;
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] nm: Enforce 32-bit width limit when printing 32-bit values
2023-12-04 22:23 ` Alan Modra
@ 2023-12-05 10:28 ` Nicolas Schier
0 siblings, 0 replies; 3+ messages in thread
From: Nicolas Schier @ 2023-12-05 10:28 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils, Johannes Nixdorf
[-- Attachment #1: Type: text/plain, Size: 6597 bytes --]
On Tue, Dec 05, 2023 at 08:53:07AM +1030, Alan Modra wrote:
> On Mon, Dec 04, 2023 at 01:03:29PM +0100, Nicolas Schier wrote:
> > Enforce printing only lower-half of 64-bit values if 32-bit print width
> > is requested.
> >
> > Printing 32-bit values >= 0x80000000 on MIPS32 causes effective output
> > of a sign-extended 64-bit value since commit 0e3c1eebb22 ("Remove use of
> > bfd_uint64_t and similar", 2022-05-27). While this might be considered
>
> Hmm, that's only partly true. Prior to commit 0e3c1eebb22 nm output
> depended on the host unsigned long when printing "negative" symbol
> values for 32-bit targets. Commit 0e3c1eebb22 made the output match
> that seen with a 64-bit host unsigned long. The fact that nm output
> changed depending on host is of course a bug.
yes, you're right.
> > appropriate for a "signed address-space", the output differs from other
> > tools from binutils, e.g. objdump, which always prints 32-bit values as
> > 32-bit values.
> >
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31096
> > Fixes: 0e3c1eebb22e ("Remove use of bfd_uint64_t and similar")
> > Signed-off-by: Nicolas Schier <n.schier@avm.de>
> > ---
> > binutils/nm.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/binutils/nm.c b/binutils/nm.c
> > index e4c8036df1b..7e42ce8f469 100644
> > --- a/binutils/nm.c
> > +++ b/binutils/nm.c
> > @@ -1821,6 +1821,9 @@ print_value (bfd *abfd ATTRIBUTE_UNUSED, bfd_vma val)
> > switch (print_width)
> > {
> > case 32:
> > + printf (print_format_string, 0xffffffff & (uint64_t) val);
> > + break;
> > +
>
> This isn't correct. At least, it changes behaviour from that prior to
> 0e3c1eebb22 for nm -td output.
ah, thanks, I wasn't aware of '-td'. Well, for decimal output it really
makes sense to be sensible with the signedness of the MIPS address
space. (Even though I have no clue, what a representation of negative
addresses is good for.)
> > case 64:
> > printf (print_format_string, (uint64_t) val);
> > break;
> > --
> > 2.40.1
>
> If we want to make nm output for 32-bit targets match that seen on
> 32-bit hosts prior to commit 0e3c1eebb22, then something like the
> following (only lightly tested) patch is needed. Please test.
Thanks a lot, works for me as expected. I built native nm for mips32,
mips64el, i386 and well as mips-cross on x86_64 and all nm versions show the
same output for my test binary:
# binutils/nm-new ../mips32-test
80000030 T test
# binutils/nm-new ../mips32-test -td
-2147483600 T test
# binutils/nm-new ../mips64el-test
0000000080000030 T test
# binutils/nm-new ../mips64el-test -td
0000002147483696 T test
Are you going to prepare a complete commit or shall I?
If it's worth:
Reviewed-by: Nicolas Schier <n.schier@avm.de>
Tested-by: Nicolas Schier <n.schier@avm.de>
Kind regards,
Nicolas
>
> diff --git a/binutils/nm.c b/binutils/nm.c
> index 54ee8182b2e..a811defba82 100644
> --- a/binutils/nm.c
> +++ b/binutils/nm.c
> @@ -169,7 +169,7 @@ static const struct output_fns formats[FORMAT_MAX] =
> /* The output format to use. */
> static const struct output_fns *format = &formats[FORMAT_DEFAULT];
> static unsigned int print_format = FORMAT_DEFAULT;
> -static const char *print_format_string = NULL;
> +static char print_format_string[10];
>
> /* Command options. */
>
> @@ -1512,37 +1512,8 @@ display_rel_file (bfd *abfd, bfd *archive_bfd)
>
> /* Construct a formatting string for printing symbol values. */
>
> -static const char *
> -get_print_format (void)
> -{
> - const char * padding;
> - if (print_format == FORMAT_POSIX || print_format == FORMAT_JUST_SYMBOLS)
> - {
> - /* POSIX compatible output does not have any padding. */
> - padding = "";
> - }
> - else if (print_width == 32)
> - {
> - padding ="08";
> - }
> - else /* print_width == 64 */
> - {
> - padding = "016";
> - }
> -
> - const char * radix = NULL;
> - switch (print_radix)
> - {
> - case 8: radix = PRIo64; break;
> - case 10: radix = PRId64; break;
> - case 16: radix = PRIx64; break;
> - }
> -
> - return concat ("%", padding, radix, NULL);
> -}
> -
> static void
> -set_print_width (bfd *file)
> +set_print_format (bfd *file)
> {
> print_width = bfd_get_arch_size (file);
>
> @@ -1559,8 +1530,43 @@ set_print_width (bfd *file)
> else
> print_width = 32;
> }
> - free ((char *) print_format_string);
> - print_format_string = get_print_format ();
> +
> + char *p = print_format_string;
> + *p++ = '%';
> + if (print_format == FORMAT_POSIX || print_format == FORMAT_JUST_SYMBOLS)
> + {
> + /* POSIX compatible output does not have any padding. */
> + }
> + else if (print_width == 32)
> + {
> + *p++ = '0';
> + *p++ = '8';
> + }
> + else /* print_width == 64 */
> + {
> + *p++ = '0';
> + *p++ = '1';
> + *p++ = '6';
> + }
> +
> + if (print_width == 32)
> + {
> + switch (print_radix)
> + {
> + case 8: strcpy (p, PRIo32); break;
> + case 10: strcpy (p, PRId32); break;
> + case 16: strcpy (p, PRIx32); break;
> + }
> + }
> + else
> + {
> + switch (print_radix)
> + {
> + case 8: strcpy (p, PRIo64); break;
> + case 10: strcpy (p, PRId64); break;
> + case 16: strcpy (p, PRIx64); break;
> + }
> + }
> }
>
> static void
> @@ -1588,7 +1594,7 @@ display_archive (bfd *file)
>
> if (bfd_check_format_matches (arfile, bfd_object, &matching))
> {
> - set_print_width (arfile);
> + set_print_format (arfile);
> format->print_archive_member (bfd_get_filename (file),
> bfd_get_filename (arfile));
> display_rel_file (arfile, file);
> @@ -1644,7 +1650,7 @@ display_file (char *filename)
> }
> else if (bfd_check_format_matches (file, bfd_object, &matching))
> {
> - set_print_width (file);
> + set_print_format (file);
> format->print_object_filename (filename);
> display_rel_file (file, NULL);
> }
> @@ -1821,6 +1827,9 @@ print_value (bfd *abfd ATTRIBUTE_UNUSED, bfd_vma val)
> switch (print_width)
> {
> case 32:
> + printf (print_format_string, (uint32_t) val);
> + break;
> +
> case 64:
> printf (print_format_string, (uint64_t) val);
> break;
>
> --
> Alan Modra
> Australia Development Lab, IBM
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-12-05 10:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-04 12:03 [PATCH] nm: Enforce 32-bit width limit when printing 32-bit values Nicolas Schier
2023-12-04 22:23 ` Alan Modra
2023-12-05 10:28 ` Nicolas Schier
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).