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

[Mike Frysinger via Gdb-patches <gdb-patches@sourceware.org>]

[-- 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 --]