[PATCH/committed] sim: rx: cast bfd_vma when printing

Message ID 20210501202929.29766-1-vapier@gentoo.org
State New
Headers show
Series
  • [PATCH/committed] sim: rx: cast bfd_vma when printing
Related show

Commit Message

Weimin Pan via Gdb-patches May 1, 2021, 8:29 p.m.
A bit of a hack, but it's what we've been doing so far when printing
bfd_vma's since bfd doesn't provide PRI helper types for us to use.
---
 sim/rx/ChangeLog | 4 ++++
 sim/rx/load.c    | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.31.1

Comments

Weimin Pan via Gdb-patches May 3, 2021, 4:20 p.m. | #1
On Sat, May 1, 2021, 15:29 Mike Frysinger via Gdb-patches <
gdb-patches@sourceware.org> wrote:

> A bit of a hack, but it's what we've been doing so far when printing

> bfd_vma's since bfd doesn't provide PRI helper types for us to use.

> ---

>  sim/rx/ChangeLog | 4 ++++

>  sim/rx/load.c    | 2 +-

>  2 files changed, 5 insertions(+), 1 deletion(-)

>

> diff --git a/sim/rx/ChangeLog b/sim/rx/ChangeLog

> index 7541c9d7141b..633ed86780c7 100644

> --- a/sim/rx/ChangeLog

> +++ b/sim/rx/ChangeLog

> @@ -1,3 +1,7 @@

> +2021-05-01  Mike Frysinger  <vapier@gentoo.org>

> +

> +       * load.c (rx_load): Cast size to long.

> +

>  2021-04-26  Mike Frysinger  <vapier@gentoo.org>

>

>         * Makefile.in (NL_TARGET): Delete.

> diff --git a/sim/rx/load.c b/sim/rx/load.c

> index a8a473346c67..b4523e12ac75 100644

> --- a/sim/rx/load.c

> +++ b/sim/rx/load.c

> @@ -151,7 +151,7 @@ rx_load (bfd *prog, host_callback *callback)

>         }

>        if (bfd_bread (buf, size, prog) != size)

>         {

> -         fprintf (stderr, "Failed to read %lx bytes\n", size);

> +         fprintf (stderr, "Failed to read %lx bytes\n", (long) size);

>


Wouldn't it be better to use "long long", especially for mingw?

Christian

          continue;
>         }

>

> --

> 2.31.1

>

>
Weimin Pan via Gdb-patches May 3, 2021, 8:57 p.m. | #2
On 03 May 2021 11:20, Christian Biesinger wrote:
> On Sat, May 1, 2021, 15:29 Mike Frysinger wrote:

> > A bit of a hack, but it's what we've been doing so far when printing

> > bfd_vma's since bfd doesn't provide PRI helper types for us to use.

> >

> > --- a/sim/rx/load.c

> > +++ b/sim/rx/load.c

> > @@ -151,7 +151,7 @@ rx_load (bfd *prog, host_callback *callback)

> >         }

> >        if (bfd_bread (buf, size, prog) != size)

> >         {

> > -         fprintf (stderr, "Failed to read %lx bytes\n", size);

> > +         fprintf (stderr, "Failed to read %lx bytes\n", (long) size);

> 

> Wouldn't it be better to use "long long", especially for mingw?


not sure why mingw is special here.  but let's ignore that.

bfd_vma printing is poorly handled in many places in sim when bfd_vma is 64-bit
but the host cpu is 32-bit (i.e. sizeof(long) == 4).  the (long) cast pattern
that i used here for printing is based on those.  i don't think sim is the only
place that does this poorly tbh, it's just what i'm focusing on.

imo picking any type to cast to and copying & pasting that everywhere is a bit
of a losing proposition.  imo we should provide a PRI constant for code to use
as that'll align with other fixed types (e.g. uint64_t).  that makes it obvious
when code is doing it correctly vs the compiler not happening to warn because
the current configuration provides typedefs that align with the hardcoded sizes.
but i don't really have the energy to take on that work.

specifically for the rx_load code, this stuff needs to get deleted entirely and
switched to the existing common/ logic.  which is why i preferred a hacky patch
here rather than diving deeper into the problem.

so you're not wrong, i'm just not investing in code that i know will die :).
-mike
Weimin Pan via Gdb-patches May 3, 2021, 9:47 p.m. | #3
On Mon, May 3, 2021 at 3:57 PM Mike Frysinger <vapier@gentoo.org> wrote:
>

> On 03 May 2021 11:20, Christian Biesinger wrote:

> > On Sat, May 1, 2021, 15:29 Mike Frysinger wrote:

> > > A bit of a hack, but it's what we've been doing so far when printing

> > > bfd_vma's since bfd doesn't provide PRI helper types for us to use.

> > >

> > > --- a/sim/rx/load.c

> > > +++ b/sim/rx/load.c

> > > @@ -151,7 +151,7 @@ rx_load (bfd *prog, host_callback *callback)

> > >         }

> > >        if (bfd_bread (buf, size, prog) != size)

> > >         {

> > > -         fprintf (stderr, "Failed to read %lx bytes\n", size);

> > > +         fprintf (stderr, "Failed to read %lx bytes\n", (long) size);

> >

> > Wouldn't it be better to use "long long", especially for mingw?

>

> not sure why mingw is special here.  but let's ignore that.


mingw (and msvc) is special because sizeof(long) is 4, even on a 64-bit system.
https://stackoverflow.com/questions/22344388/size-of-long-int-and-int-in-c-showing-4-bytes

> bfd_vma printing is poorly handled in many places in sim when bfd_vma is 64-bit

> but the host cpu is 32-bit (i.e. sizeof(long) == 4).  the (long) cast pattern

> that i used here for printing is based on those.  i don't think sim is the only

> place that does this poorly tbh, it's just what i'm focusing on.


But if other places already do that, then eh...

Christian
Weimin Pan via Gdb-patches May 4, 2021, 12:47 a.m. | #4
On 03 May 2021 16:47, Christian Biesinger via Gdb-patches wrote:
> On Mon, May 3, 2021 at 3:57 PM Mike Frysinger wrote:

> > On 03 May 2021 11:20, Christian Biesinger wrote:

> > > On Sat, May 1, 2021, 15:29 Mike Frysinger wrote:

> > > > A bit of a hack, but it's what we've been doing so far when printing

> > > > bfd_vma's since bfd doesn't provide PRI helper types for us to use.

> > > >

> > > > --- a/sim/rx/load.c

> > > > +++ b/sim/rx/load.c

> > > > @@ -151,7 +151,7 @@ rx_load (bfd *prog, host_callback *callback)

> > > >         }

> > > >        if (bfd_bread (buf, size, prog) != size)

> > > >         {

> > > > -         fprintf (stderr, "Failed to read %lx bytes\n", size);

> > > > +         fprintf (stderr, "Failed to read %lx bytes\n", (long) size);

> > >

> > > Wouldn't it be better to use "long long", especially for mingw?

> >

> > not sure why mingw is special here.  but let's ignore that.

> 

> mingw (and msvc) is special because sizeof(long) is 4, even on a 64-bit system.

> https://stackoverflow.com/questions/22344388/size-of-long-int-and-int-in-c-showing-4-bytes


we have ILP32 models on mips/n32 [2000], x86_64/x32 [2011], and aarch64 [2016],
so i don't think mingw does anything that unique anymore.

we've long known that long is inappropriate for things that could be 64-bit.
the trouble is when we half implement the suite like just coming up with a
typedef and skimping on printf formats :(.
-mike
Weimin Pan via Gdb-patches May 4, 2021, 2:54 a.m. | #5
On 03 May 2021 20:47, Mike Frysinger via Gdb-patches wrote:
> On 03 May 2021 16:47, Christian Biesinger via Gdb-patches wrote:

> > On Mon, May 3, 2021 at 3:57 PM Mike Frysinger wrote:

> > > On 03 May 2021 11:20, Christian Biesinger wrote:

> > > > On Sat, May 1, 2021, 15:29 Mike Frysinger wrote:

> > > > > A bit of a hack, but it's what we've been doing so far when printing

> > > > > bfd_vma's since bfd doesn't provide PRI helper types for us to use.

> > > > >

> > > > > --- a/sim/rx/load.c

> > > > > +++ b/sim/rx/load.c

> > > > > @@ -151,7 +151,7 @@ rx_load (bfd *prog, host_callback *callback)

> > > > >         }

> > > > >        if (bfd_bread (buf, size, prog) != size)

> > > > >         {

> > > > > -         fprintf (stderr, "Failed to read %lx bytes\n", size);

> > > > > +         fprintf (stderr, "Failed to read %lx bytes\n", (long) size);

> > > >

> > > > Wouldn't it be better to use "long long", especially for mingw?

> > >

> > > not sure why mingw is special here.  but let's ignore that.

> > 

> > mingw (and msvc) is special because sizeof(long) is 4, even on a 64-bit system.

> > https://stackoverflow.com/questions/22344388/size-of-long-int-and-int-in-c-showing-4-bytes

> 

> we have ILP32 models on mips/n32 [2000], x86_64/x32 [2011], and aarch64 [2016],

> so i don't think mingw does anything that unique anymore.

> 

> we've long known that long is inappropriate for things that could be 64-bit.

> the trouble is when we half implement the suite like just coming up with a

> typedef and skimping on printf formats :(.


looks like i'm just bad at grepping for things i don't know about.  i could
find bad examples, but that's because i knew the form they took.  a patch on
the binutils list points out the format already exists, albeit with a more
verbose name: BFD_VMA_FMT.  so i can clean up the sim code to use that.
-mike

Patch

diff --git a/sim/rx/ChangeLog b/sim/rx/ChangeLog
index 7541c9d7141b..633ed86780c7 100644
--- a/sim/rx/ChangeLog
+++ b/sim/rx/ChangeLog
@@ -1,3 +1,7 @@ 
+2021-05-01  Mike Frysinger  <vapier@gentoo.org>
+
+	* load.c (rx_load): Cast size to long.
+
 2021-04-26  Mike Frysinger  <vapier@gentoo.org>
 
 	* Makefile.in (NL_TARGET): Delete.
diff --git a/sim/rx/load.c b/sim/rx/load.c
index a8a473346c67..b4523e12ac75 100644
--- a/sim/rx/load.c
+++ b/sim/rx/load.c
@@ -151,7 +151,7 @@  rx_load (bfd *prog, host_callback *callback)
 	}
       if (bfd_bread (buf, size, prog) != size)
 	{
-	  fprintf (stderr, "Failed to read %lx bytes\n", size);
+	  fprintf (stderr, "Failed to read %lx bytes\n", (long) size);
 	  continue;
 	}