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

[Gedare Bloom]

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*)))
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