On Sun, Mar 21, 2021 at 6:46 PM Chris Johns <chr...@rtems.org> wrote: > > > > 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. 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 _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
