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

[Christian Biesinger <cbiesinger@google.com>]

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