On Mon, Mar 22, 2021 at 12:54 PM Joel Sherrill <j...@rtems.org> wrote:
> I posted to the gdb mailing list and this is the response: > > "If the inferior is using 64-bit addresses, then the remote protocol will > also use 64-bit addresses. If we have a 32-bit inferior running on > aarch64 hardware, we'll have 32-bit addresses over the remote protocol > as well. > > Even when we're using 64-bit addresses, the remote protocol may not pad > it with zeroes to make it 64-bit, but it should still be handled as > 64-bit." > > Unfortunately, I think this means that everywhere the debugger code > uses a decode (is there an encode?) uint to decode an address that > code needs to change to decode an address (e.g. uintptr) where one is > expected in the protocol. And all target addresses in the debugger need > to change to uintptr_t. > Luis from gdb just followed up and suggested we always treat it as 64-bit and cast to 32-bits. I think uintptr_t is the right answer but I wanted to pass that along. > > Chris commented on a debugger internal type needed review also. > > Stephen.. your fix is probably more right than wrong but it needs to > be broader. See what you can do. Kinsey and I can help with this as > you need it. > > You will definitely have to test the gdb server code after this with the > BBB. > > --joel > > On Mon, Mar 22, 2021 at 11:45 AM Gedare Bloom <ged...@rtems.org> wrote: > >> 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
