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

[Chris Johns]

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.

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.

Chris