[PATCH] sim: mn10300: clean up pointer casts

[PATCH] sim: mn10300: clean up pointer casts

[Mike Frysinger]

The void *data field is used to past arbitrary data between event
handlers, and these are using it to pass an enum.  Fix up the casts
to avoid using (long) to cast to/from pointers since there is no
guarantee that's the right size.
---
 sim/mn10300/dv-mn103ser.c |  6 +++---
 sim/mn10300/dv-mn103tim.c | 12 ++++++------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/sim/mn10300/dv-mn103ser.c b/sim/mn10300/dv-mn103ser.c
index 3465954f75f7..d2140e22c196 100644
--- a/sim/mn10300/dv-mn103ser.c
+++ b/sim/mn10300/dv-mn103ser.c
@@ -238,7 +238,7 @@ do_polling_event (struct hw *me,
 {
   SIM_DESC sd = hw_system (me);
   struct mn103ser *serial = hw_data(me);
-  long serial_reg = (long) data;
+  long serial_reg = (uintptr_t) data;
   char c;
   int count, status;
 
@@ -280,7 +280,7 @@ do_polling_event (struct hw *me,
   /* Schedule next polling event */
   serial->device[serial_reg].event
     = hw_event_queue_schedule (me, 1000,
-			       do_polling_event, (void *)serial_reg);
+			       do_polling_event, (void *)(uintptr_t)serial_reg);
 
 }
 
@@ -424,7 +424,7 @@ read_status_reg (struct hw *me,
       serial->device[serial_reg].event
 	= hw_event_queue_schedule (me, 1000,
 				   do_polling_event,
-				   (void *) (long) serial_reg);
+				   (void *)(uintptr_t)serial_reg);
     }
 
   if ( nr_bytes == 1 )
diff --git a/sim/mn10300/dv-mn103tim.c b/sim/mn10300/dv-mn103tim.c
index ad9c24bbc6e8..76f87380d1d8 100644
--- a/sim/mn10300/dv-mn103tim.c
+++ b/sim/mn10300/dv-mn103tim.c
@@ -570,7 +570,7 @@ do_counter_event (struct hw *me,
 		  void *data)
 {
   struct mn103tim *timers = hw_data(me);
-  long timer_nr = (long) data;
+  long timer_nr = (uintptr_t) data;
   int next_timer;
 
   /* Check if counting is still enabled. */
@@ -596,7 +596,7 @@ do_counter_event (struct hw *me,
       /* FIX: Check if div_ratio has changed and if it's now 0. */
       timers->timer[timer_nr].event
 	= hw_event_queue_schedule (me, timers->timer[timer_nr].div_ratio,
-				   do_counter_event, (void *)timer_nr);
+				   do_counter_event, (void *)(uintptr_t)timer_nr);
     }
   else
     {
@@ -611,7 +611,7 @@ do_counter6_event (struct hw *me,
 		  void *data)
 {
   struct mn103tim *timers = hw_data(me);
-  long timer_nr = (long) data;
+  long timer_nr = (uintptr_t) data;
   int next_timer;
 
   /* Check if counting is still enabled. */
@@ -625,7 +625,7 @@ do_counter6_event (struct hw *me,
       /* FIX: Check if div_ratio has changed and if it's now 0. */
       timers->timer[timer_nr].event
 	= hw_event_queue_schedule (me, timers->timer[timer_nr].div_ratio,
-				   do_counter6_event, (void *)timer_nr);
+				   do_counter6_event, (void *)(uintptr_t)timer_nr);
     }
   else
     {
@@ -808,7 +808,7 @@ write_mode_reg (struct hw *me,
 	      timers->timer[timer_nr].event
 		= hw_event_queue_schedule(me, div_ratio,
 					  do_counter_event,
-					  (void *)(timer_nr)); 
+					  (void *)(uintptr_t)timer_nr);
 	    }
 	}
     }
@@ -908,7 +908,7 @@ write_tm6md (struct hw *me,
 	  timers->timer[timer_nr].event
 	    = hw_event_queue_schedule(me, div_ratio,
 				      do_counter6_event,
-				      (void *)(timer_nr)); 
+				      (void *)(uintptr_t)timer_nr);
 	}
     }
   else
-- 
2.33.0

Re: [PATCH] sim: mn10300: clean up pointer casts

[Christian Biesinger]

On Sun, Oct 31, 2021 at 11:45 PM Mike Frysinger via Gdb-patches
<gdb-patches@sourceware.org> wrote:
> --- a/sim/mn10300/dv-mn103ser.c
> +++ b/sim/mn10300/dv-mn103ser.c
> @@ -238,7 +238,7 @@ do_polling_event (struct hw *me,
>  {
>    SIM_DESC sd = hw_system (me);
>    struct mn103ser *serial = hw_data(me);
> -  long serial_reg = (long) data;
> +  long serial_reg = (uintptr_t) data;

Shouldn't you change the variable type too? Otherwise this patch seems
to be somewhat pointless?
    if ( nr_bytes == 1 )

> diff --git a/sim/mn10300/dv-mn103tim.c b/sim/mn10300/dv-mn103tim.c
> index ad9c24bbc6e8..76f87380d1d8 100644
> --- a/sim/mn10300/dv-mn103tim.c
> +++ b/sim/mn10300/dv-mn103tim.c
> @@ -570,7 +570,7 @@ do_counter_event (struct hw *me,
>                   void *data)
>  {
>    struct mn103tim *timers = hw_data(me);
> -  long timer_nr = (long) data;
> +  long timer_nr = (uintptr_t) data;

Here too.

> @@ -611,7 +611,7 @@ do_counter6_event (struct hw *me,
>                   void *data)
>  {
>    struct mn103tim *timers = hw_data(me);
> -  long timer_nr = (long) data;
> +  long timer_nr = (uintptr_t) data;

And here.

Christian

Re: [PATCH] sim: mn10300: clean up pointer casts

[Mike Frysinger]

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

On 01 Nov 2021 12:07, Christian Biesinger wrote:
> On Sun, Oct 31, 2021 at 11:45 PM Mike Frysinger via Gdb-patches
> <gdb-patches@sourceware.org> wrote:
> > --- a/sim/mn10300/dv-mn103ser.c
> > +++ b/sim/mn10300/dv-mn103ser.c
> > @@ -238,7 +238,7 @@ do_polling_event (struct hw *me,
> >  {
> >    SIM_DESC sd = hw_system (me);
> >    struct mn103ser *serial = hw_data(me);
> > -  long serial_reg = (long) data;
> > +  long serial_reg = (uintptr_t) data;
> 
> Shouldn't you change the variable type too?

the types are already confused, but long here is fine as any other.  uintptr_t
is not really any more correct.  serial_reg comes in as an unsigned_word via
the memory read/write callbacks, (encoded) cast to void*, and (decoded) cast
to long.  the memory read/write callbacks verify the values are always within
the serial_register_types enum range which looks like [0,16] range, so any of
these types work.

> Otherwise this patch seems to be somewhat pointless?
>     if ( nr_bytes == 1 )

i don't know which one you mean, or why you think any of them are pointless.
the nr_bytes checks are in the read/write code paths of the memory mapped
register callbacks.  so nr_bytes is needed & correct there.

the uintptr_t casts happen when passing information to the scheduled callback
which is after the nr_bytes checks.

i'm not anti trying to clean up these device models, i just don't have any
personal interest in them, so i'm not inclined to spend time on them anymore
than compiler warnings/failures force me to, or to make progress in unifying
all the ports.  the lack of test coverage also makes me a bit inclined to not
get too invasive if i can avoid it.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] sim: mn10300: clean up pointer casts

[Christian Biesinger]

On Mon, Nov 1, 2021 at 12:22 PM Mike Frysinger <vapier@gentoo.org> wrote:
>
> On 01 Nov 2021 12:07, Christian Biesinger wrote:
> > On Sun, Oct 31, 2021 at 11:45 PM Mike Frysinger via Gdb-patches
> > <gdb-patches@sourceware.org> wrote:
> > > --- a/sim/mn10300/dv-mn103ser.c
> > > +++ b/sim/mn10300/dv-mn103ser.c
> > > @@ -238,7 +238,7 @@ do_polling_event (struct hw *me,
> > >  {
> > >    SIM_DESC sd = hw_system (me);
> > >    struct mn103ser *serial = hw_data(me);
> > > -  long serial_reg = (long) data;
> > > +  long serial_reg = (uintptr_t) data;
> >
> > Shouldn't you change the variable type too?
>
> the types are already confused, but long here is fine as any other.  uintptr_t
> is not really any more correct.  serial_reg comes in as an unsigned_word via
> the memory read/write callbacks, (encoded) cast to void*, and (decoded) cast
> to long.  the memory read/write callbacks verify the values are always within
> the serial_register_types enum range which looks like [0,16] range, so any of
> these types work.
>
> > Otherwise this patch seems to be somewhat pointless?
> >     if ( nr_bytes == 1 )
>
> i don't know which one you mean, or why you think any of them are pointless.
> the nr_bytes checks are in the read/write code paths of the memory mapped
> register callbacks.  so nr_bytes is needed & correct there.

I forgot to delete that nr_bytes line; the pointless comment was about
casting to uintprtr_t but still storing the result in a long. Why not
keep it as a cast to long then?

Christian

Re: [PATCH] sim: mn10300: clean up pointer casts

[Mike Frysinger]

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

On 01 Nov 2021 14:28, Christian Biesinger via Gdb-patches wrote:
> On Mon, Nov 1, 2021 at 12:22 PM Mike Frysinger <vapier@gentoo.org> wrote:
> > On 01 Nov 2021 12:07, Christian Biesinger wrote:
> > > On Sun, Oct 31, 2021 at 11:45 PM Mike Frysinger via Gdb-patches
> > > <gdb-patches@sourceware.org> wrote:
> > > > --- a/sim/mn10300/dv-mn103ser.c
> > > > +++ b/sim/mn10300/dv-mn103ser.c
> > > > @@ -238,7 +238,7 @@ do_polling_event (struct hw *me,
> > > >  {
> > > >    SIM_DESC sd = hw_system (me);
> > > >    struct mn103ser *serial = hw_data(me);
> > > > -  long serial_reg = (long) data;
> > > > +  long serial_reg = (uintptr_t) data;
> > >
> > > Shouldn't you change the variable type too?
> >
> > the types are already confused, but long here is fine as any other.  uintptr_t
> > is not really any more correct.  serial_reg comes in as an unsigned_word via
> > the memory read/write callbacks, (encoded) cast to void*, and (decoded) cast
> > to long.  the memory read/write callbacks verify the values are always within
> > the serial_register_types enum range which looks like [0,16] range, so any of
> > these types work.
> >
> > > Otherwise this patch seems to be somewhat pointless?
> > >     if ( nr_bytes == 1 )
> >
> > i don't know which one you mean, or why you think any of them are pointless.
> > the nr_bytes checks are in the read/write code paths of the memory mapped
> > register callbacks.  so nr_bytes is needed & correct there.
> 
> I forgot to delete that nr_bytes line; the pointless comment was about
> casting to uintprtr_t but still storing the result in a long. Why not
> keep it as a cast to long then?

that was the point of the patch in the first place :).  i prob should have
included the compiler errors in the log, but basically any target where the
sizeof(long) != sizeof(void*) will generate an error.  LLP64 systems is where
you'll find this, and most notably, 64-bit Windows does it.
warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]

since sizeof(uintptr_t) is guaranteed to equal sizeof(void*), it's the "right"
way to convert integers to/from pointers.  in this case, the event framework
is using void* as an opaque storage, and this particular model is (ab)using
that to pass values rather than pointers to larger storage.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] sim: mn10300: clean up pointer casts

[Christian Biesinger]

On Mon, Nov 1, 2021 at 2:59 PM Mike Frysinger <vapier@gentoo.org> wrote:
>
> On 01 Nov 2021 14:28, Christian Biesinger via Gdb-patches wrote:
> > On Mon, Nov 1, 2021 at 12:22 PM Mike Frysinger <vapier@gentoo.org> wrote:
> > > On 01 Nov 2021 12:07, Christian Biesinger wrote:
> > > > On Sun, Oct 31, 2021 at 11:45 PM Mike Frysinger via Gdb-patches
> > > > <gdb-patches@sourceware.org> wrote:
> > > > > --- a/sim/mn10300/dv-mn103ser.c
> > > > > +++ b/sim/mn10300/dv-mn103ser.c
> > > > > @@ -238,7 +238,7 @@ do_polling_event (struct hw *me,
> > > > >  {
> > > > >    SIM_DESC sd = hw_system (me);
> > > > >    struct mn103ser *serial = hw_data(me);
> > > > > -  long serial_reg = (long) data;
> > > > > +  long serial_reg = (uintptr_t) data;
> > > >
> > > > Shouldn't you change the variable type too?
> > >
> > > the types are already confused, but long here is fine as any other.  uintptr_t
> > > is not really any more correct.  serial_reg comes in as an unsigned_word via
> > > the memory read/write callbacks, (encoded) cast to void*, and (decoded) cast
> > > to long.  the memory read/write callbacks verify the values are always within
> > > the serial_register_types enum range which looks like [0,16] range, so any of
> > > these types work.
> > >
> > > > Otherwise this patch seems to be somewhat pointless?
> > > >     if ( nr_bytes == 1 )
> > >
> > > i don't know which one you mean, or why you think any of them are pointless.
> > > the nr_bytes checks are in the read/write code paths of the memory mapped
> > > register callbacks.  so nr_bytes is needed & correct there.
> >
> > I forgot to delete that nr_bytes line; the pointless comment was about
> > casting to uintprtr_t but still storing the result in a long. Why not
> > keep it as a cast to long then?
>
> that was the point of the patch in the first place :).  i prob should have
> included the compiler errors in the log, but basically any target where the
> sizeof(long) != sizeof(void*) will generate an error.  LLP64 systems is where
> you'll find this, and most notably, 64-bit Windows does it.
> warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
>
> since sizeof(uintptr_t) is guaranteed to equal sizeof(void*), it's the "right"
> way to convert integers to/from pointers.  in this case, the event framework
> is using void* as an opaque storage, and this particular model is (ab)using
> that to pass values rather than pointers to larger storage.

I see, so what the patch does is to cast to uintptr_t to avoid the
compile error about losing precision and rely on an implicit
conversion to long?

My suggestion would be to make that a bit clearer in the commit
message and perhaps explicitly convert from uintptr_t to long.

Christian

Re: [PATCH] sim: mn10300: clean up pointer casts

[Mike Frysinger]

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

On 01 Nov 2021 15:35, Christian Biesinger via Gdb-patches wrote:
> On Mon, Nov 1, 2021 at 2:59 PM Mike Frysinger <vapier@gentoo.org> wrote:
> >
> > On 01 Nov 2021 14:28, Christian Biesinger via Gdb-patches wrote:
> > > On Mon, Nov 1, 2021 at 12:22 PM Mike Frysinger <vapier@gentoo.org> wrote:
> > > > On 01 Nov 2021 12:07, Christian Biesinger wrote:
> > > > > On Sun, Oct 31, 2021 at 11:45 PM Mike Frysinger via Gdb-patches
> > > > > <gdb-patches@sourceware.org> wrote:
> > > > > > --- a/sim/mn10300/dv-mn103ser.c
> > > > > > +++ b/sim/mn10300/dv-mn103ser.c
> > > > > > @@ -238,7 +238,7 @@ do_polling_event (struct hw *me,
> > > > > >  {
> > > > > >    SIM_DESC sd = hw_system (me);
> > > > > >    struct mn103ser *serial = hw_data(me);
> > > > > > -  long serial_reg = (long) data;
> > > > > > +  long serial_reg = (uintptr_t) data;
> > > > >
> > > > > Shouldn't you change the variable type too?
> > > >
> > > > the types are already confused, but long here is fine as any other.  uintptr_t
> > > > is not really any more correct.  serial_reg comes in as an unsigned_word via
> > > > the memory read/write callbacks, (encoded) cast to void*, and (decoded) cast
> > > > to long.  the memory read/write callbacks verify the values are always within
> > > > the serial_register_types enum range which looks like [0,16] range, so any of
> > > > these types work.
> > > >
> > > > > Otherwise this patch seems to be somewhat pointless?
> > > > >     if ( nr_bytes == 1 )
> > > >
> > > > i don't know which one you mean, or why you think any of them are pointless.
> > > > the nr_bytes checks are in the read/write code paths of the memory mapped
> > > > register callbacks.  so nr_bytes is needed & correct there.
> > >
> > > I forgot to delete that nr_bytes line; the pointless comment was about
> > > casting to uintprtr_t but still storing the result in a long. Why not
> > > keep it as a cast to long then?
> >
> > that was the point of the patch in the first place :).  i prob should have
> > included the compiler errors in the log, but basically any target where the
> > sizeof(long) != sizeof(void*) will generate an error.  LLP64 systems is where
> > you'll find this, and most notably, 64-bit Windows does it.
> > warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
> >
> > since sizeof(uintptr_t) is guaranteed to equal sizeof(void*), it's the "right"
> > way to convert integers to/from pointers.  in this case, the event framework
> > is using void* as an opaque storage, and this particular model is (ab)using
> > that to pass values rather than pointers to larger storage.
> 
> I see, so what the patch does is to cast to uintptr_t to avoid the
> compile error about losing precision and rely on an implicit
> conversion to long?

right

> My suggestion would be to make that a bit clearer in the commit

sure, sorry about that.  i'll try to include more details in the future.

> message and perhaps explicitly convert from uintptr_t to long.

eh -- see above about my investment interest in these models :).
i don't think implicit conversions between integer types are a
bad thing to avoid in general, especially given the narrow scope.
-mike

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]