On 21/3/21 2:26 am, Joel Sherrill wrote: > > > On Sat, Mar 20, 2021, 1:21 AM Gedare Bloom <ged...@rtems.org > <mailto:ged...@rtems.org>> wrote: > > Hi Stephen, Joel: > > On Fri, Mar 19, 2021 at 9:35 AM Stephen Clark <stephen.cl...@oarcorp.com > <mailto:stephen.cl...@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 _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel