On Mon, Mar 22, 2021 at 10:44 AM Gedare Bloom <ged...@rtems.org> wrote: > > 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*))) 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 _______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
