[PATCH v3 1/3] rtems-debugger: Fixed 32bit pointers

[Gedare Bloom]

On Mon, Mar 22, 2021 at 10:44 AM Gedare Bloom <gedare at rtems.org> wrote:
>
> On Sun, Mar 21, 2021 at 6:46 PM Chris Johns <chrisj at rtems.org> wrote:
> >
> >
> >
> > On 21/3/21 2:26 am, Joel Sherrill wrote:
> > >
> > >
> > > On Sat, Mar 20, 2021, 1:21 AM Gedare Bloom <gedare at rtems.org
> > > <mailto:gedare at rtems.org>> wrote:
> > >
> > >     Hi Stephen, Joel:
> > >
> > >     On Fri, Mar 19, 2021 at 9:35 AM Stephen Clark <stephen.clark at oarcorp.com
> > >     <mailto:stephen.clark at oarcorp.com>> wrote:
> > >     >
> > >     > Using 32bit types like uint32_t for pointers creates issues on 64 bit
> > >     > architectures like AArch64. Replaced occurrences of these with uintptr_t,
> > >     > which will work for both 32 and 64 bit architectures.
> > >     > ---
> > >     >  cpukit/libdebugger/rtems-debugger-server.c | 4 ++--
> > >     >  cpukit/libdebugger/rtems-debugger-target.c | 2 +-
> > >     >  cpukit/libdebugger/rtems-debugger-target.h | 2 +-
> > >     >  3 files changed, 4 insertions(+), 4 deletions(-)
> > >     >
> > >     > diff --git a/cpukit/libdebugger/rtems-debugger-server.c
> > >     b/cpukit/libdebugger/rtems-debugger-server.c
> > >     > index 975ec23a30..f8c485a794 100644
> > >     > --- a/cpukit/libdebugger/rtems-debugger-server.c
> > >     > +++ b/cpukit/libdebugger/rtems-debugger-server.c
> > >     > @@ -1438,7 +1438,7 @@ remote_read_memory(uint8_t* buffer, int size)
> > >     >    if (comma == NULL)
> > >     >      remote_packet_out_str(r_E01);
> > >     >    else {
> > >     > -    DB_UINT addr;
> > >     > +    uintptr_t addr;
> > >     >      DB_UINT length;
> > >     >      int     r;
> > >     >      addr = hex_decode_uint(&buffer[1]);
> > >
> > >     I still have concerns that just changing the variable types is not
> > >     addressing any possible fundamental type errors that get hidden by
> > >     later type coercion, for example, here we see addr = hex_decode_uint()
> > >     that returns type DB_UINT, so now we have a type mismatch in 64-bit
> > >     targets. Now, maybe it is fine, because the addresses are limited to
> > >     32-bits in practice, but I would like some discussion of the analysis
> > >     that has been done on these type changes.
> > >
> > >
> > > I commented earlier in this thread that Chris needs to comment on the gdb
> > > protocol. Switching to uintptr_t should be correct but, as you point out,
> > > whether or not the gdb protocol uses 64 bit addresses in the messages is unknown
> > > to us both.
> > >
> > > Looking at the gdb manual, it seems to use addr a lot without explaining this point.
> > >
> > > My money says there needs to be a decode address which returns a uintptr_t. And
> > > that combined with this patch and a review of where the variables are used is
> > > needed.
> > >
> > > I suspect the answer is in something like remote.c in the gdb source.
> > >
> > > But we really need guidance from Chris.
> >
> > I have not looked at 64bit support in detail for this code. I add the DB_INT to
> > provide a type that might help but at the time my focus was getting 32bit to
> > work. Back then I did not know if a single standards based type could fill the
> > role or a special type was needed and I am no wiser so if those who are working
> > with 64bit targets say it works for both 32bit and 64bit then I will trust their
> > judgement and DB_INT can be removed and all spots updated. Note, DB_INT needs to
> > be removed for the change to make sense.
> Stephen, Joel: Please consider propagating conversion of DB_INT into
> uintptr_t if it makes sense.
>
> From a standard types perspective, uintptr_t is guaranteed to be large
> enough to hold an integer or pointer type, we can usually think of it
> as:
>  MIN(sizeof(unsigned int), sizeof(sizeof(void*)))
sorry, just one sizeof in the second term.

> Some history/background: The uintptr_t type is exactly meant to be
> used where addresses are stored in integer type variables, and may be
> cast back/forth with pointer types. One thing you can do with this
> type is integer arithmetic as opposed to pointer arithmetic. In older
> versions of systems software, it was common practice to use the
> 'unsigned long' type for this purpose, but that breaks on 64-bit
> architectures because long is only guaranteed to be a minimum of 4
> bytes.
>
> >
> > Gedare, I agree with your observation. I think the dependent code needs to be
> > checked to make sure further problems are not hidden and we avoid creating new
> > problems that are not visible.
> >
> > I would love a back end for the A52 etc but I am realistic that it may not
> > happen however if a function like hex_decode_uint needs to be updated I suggest
> > it is.
> >
> > Joel, if your team at OAR can run a qemu 64 bit arm debug session then I suggest
> > you turn on the remote protocol trace, inspect the data being exchanged and
> > check if the decoder will work. The changes seem to look OK but until a test on
> > 32bit ARM hardware like a beaglebone black happens I cannot say.
> >
> +1
>
> We should be careful about making these "64-bit clean" modifications
> that just get the compiler to pass. I had to do that for example in
> the legacy networking stack during development of sparc64, because I
> didn't have a functional stack to test it with, so eventually someone
> else (Sebastian, I think) had to debug why those changes didn't work
> right. So, if the 64-bit cleaning changes aren't tested, why they
> weren't tested must be discussed, and notes should be left behind in
> case someone later tries to run the code and it doesn't work as
> expected. Much better would be to test the changes if it is at all
> possible.
>
> Gedare