<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 22, 2021 at 12:54 PM Joel Sherrill <<a href="mailto:joel@rtems.org">joel@rtems.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">I posted to the gdb mailing list and this is the response:<br><br>"If the inferior is using 64-bit addresses, then the remote protocol will<br>also use 64-bit addresses. If we have a 32-bit inferior running on<br>aarch64 hardware, we'll have 32-bit addresses over the remote protocol<br>as well.<br><br>Even when we're using 64-bit addresses, the remote protocol may not pad<br>it with zeroes to make it 64-bit, but it should still be handled as 64-bit."<br><div><br></div><div>Unfortunately, I think this means that everywhere the debugger code </div><div>uses a decode (is there an encode?) uint to decode an address that</div><div>code needs to change to decode an address (e.g. uintptr) where one is </div><div>expected in the protocol. And all target addresses in the debugger need </div><div>to change to uintptr_t.</div></div></blockquote><div><br></div><div>Luis from gdb just followed up and suggested we always treat it as</div><div>64-bit and cast to 32-bits. I think uintptr_t is the right answer but I </div><div>wanted to pass that along.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><br>Chris commented on a debugger internal type needed review also.</div><div><br></div><div>Stephen.. your fix is probably more right than wrong but it needs to</div><div>be broader. See what you can do. Kinsey and I can help with this as</div><div>you need it. <br><br>You will definitely have to test the gdb server code after this with the</div><div>BBB.</div><div><br></div><div>--joel</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 22, 2021 at 11:45 AM Gedare Bloom <<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, Mar 22, 2021 at 10:44 AM Gedare Bloom <<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a>> wrote:<br>
><br>
> On Sun, Mar 21, 2021 at 6:46 PM Chris Johns <<a href="mailto:chrisj@rtems.org" target="_blank">chrisj@rtems.org</a>> wrote:<br>
> ><br>
> ><br>
> ><br>
> > On 21/3/21 2:26 am, Joel Sherrill wrote:<br>
> > ><br>
> > ><br>
> > > On Sat, Mar 20, 2021, 1:21 AM Gedare Bloom <<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a><br>
> > > <mailto:<a href="mailto:gedare@rtems.org" target="_blank">gedare@rtems.org</a>>> wrote:<br>
> > ><br>
> > > Hi Stephen, Joel:<br>
> > ><br>
> > > On Fri, Mar 19, 2021 at 9:35 AM Stephen Clark <<a href="mailto:stephen.clark@oarcorp.com" target="_blank">stephen.clark@oarcorp.com</a><br>
> > > <mailto:<a href="mailto:stephen.clark@oarcorp.com" target="_blank">stephen.clark@oarcorp.com</a>>> wrote:<br>
> > > ><br>
> > > > Using 32bit types like uint32_t for pointers creates issues on 64 bit<br>
> > > > architectures like AArch64. Replaced occurrences of these with uintptr_t,<br>
> > > > which will work for both 32 and 64 bit architectures.<br>
> > > > ---<br>
> > > > cpukit/libdebugger/rtems-debugger-server.c | 4 ++--<br>
> > > > cpukit/libdebugger/rtems-debugger-target.c | 2 +-<br>
> > > > cpukit/libdebugger/rtems-debugger-target.h | 2 +-<br>
> > > > 3 files changed, 4 insertions(+), 4 deletions(-)<br>
> > > ><br>
> > > > diff --git a/cpukit/libdebugger/rtems-debugger-server.c<br>
> > > b/cpukit/libdebugger/rtems-debugger-server.c<br>
> > > > index 975ec23a30..f8c485a794 100644<br>
> > > > --- a/cpukit/libdebugger/rtems-debugger-server.c<br>
> > > > +++ b/cpukit/libdebugger/rtems-debugger-server.c<br>
> > > > @@ -1438,7 +1438,7 @@ remote_read_memory(uint8_t* buffer, int size)<br>
> > > > if (comma == NULL)<br>
> > > > remote_packet_out_str(r_E01);<br>
> > > > else {<br>
> > > > - DB_UINT addr;<br>
> > > > + uintptr_t addr;<br>
> > > > DB_UINT length;<br>
> > > > int r;<br>
> > > > addr = hex_decode_uint(&buffer[1]);<br>
> > ><br>
> > > I still have concerns that just changing the variable types is not<br>
> > > addressing any possible fundamental type errors that get hidden by<br>
> > > later type coercion, for example, here we see addr = hex_decode_uint()<br>
> > > that returns type DB_UINT, so now we have a type mismatch in 64-bit<br>
> > > targets. Now, maybe it is fine, because the addresses are limited to<br>
> > > 32-bits in practice, but I would like some discussion of the analysis<br>
> > > that has been done on these type changes.<br>
> > ><br>
> > ><br>
> > > I commented earlier in this thread that Chris needs to comment on the gdb<br>
> > > protocol. Switching to uintptr_t should be correct but, as you point out,<br>
> > > whether or not the gdb protocol uses 64 bit addresses in the messages is unknown<br>
> > > to us both.<br>
> > ><br>
> > > Looking at the gdb manual, it seems to use addr a lot without explaining this point.<br>
> > ><br>
> > > My money says there needs to be a decode address which returns a uintptr_t. And<br>
> > > that combined with this patch and a review of where the variables are used is<br>
> > > needed.<br>
> > ><br>
> > > I suspect the answer is in something like remote.c in the gdb source.<br>
> > ><br>
> > > But we really need guidance from Chris.<br>
> ><br>
> > I have not looked at 64bit support in detail for this code. I add the DB_INT to<br>
> > provide a type that might help but at the time my focus was getting 32bit to<br>
> > work. Back then I did not know if a single standards based type could fill the<br>
> > role or a special type was needed and I am no wiser so if those who are working<br>
> > with 64bit targets say it works for both 32bit and 64bit then I will trust their<br>
> > judgement and DB_INT can be removed and all spots updated. Note, DB_INT needs to<br>
> > be removed for the change to make sense.<br>
> Stephen, Joel: Please consider propagating conversion of DB_INT into<br>
> uintptr_t if it makes sense.<br>
><br>
> From a standard types perspective, uintptr_t is guaranteed to be large<br>
> enough to hold an integer or pointer type, we can usually think of it<br>
> as:<br>
> MIN(sizeof(unsigned int), sizeof(sizeof(void*)))<br>
sorry, just one sizeof in the second term.<br>
<br>
> Some history/background: The uintptr_t type is exactly meant to be<br>
> used where addresses are stored in integer type variables, and may be<br>
> cast back/forth with pointer types. One thing you can do with this<br>
> type is integer arithmetic as opposed to pointer arithmetic. In older<br>
> versions of systems software, it was common practice to use the<br>
> 'unsigned long' type for this purpose, but that breaks on 64-bit<br>
> architectures because long is only guaranteed to be a minimum of 4<br>
> bytes.<br>
><br>
> ><br>
> > Gedare, I agree with your observation. I think the dependent code needs to be<br>
> > checked to make sure further problems are not hidden and we avoid creating new<br>
> > problems that are not visible.<br>
> ><br>
> > I would love a back end for the A52 etc but I am realistic that it may not<br>
> > happen however if a function like hex_decode_uint needs to be updated I suggest<br>
> > it is.<br>
> ><br>
> > Joel, if your team at OAR can run a qemu 64 bit arm debug session then I suggest<br>
> > you turn on the remote protocol trace, inspect the data being exchanged and<br>
> > check if the decoder will work. The changes seem to look OK but until a test on<br>
> > 32bit ARM hardware like a beaglebone black happens I cannot say.<br>
> ><br>
> +1<br>
><br>
> We should be careful about making these "64-bit clean" modifications<br>
> that just get the compiler to pass. I had to do that for example in<br>
> the legacy networking stack during development of sparc64, because I<br>
> didn't have a functional stack to test it with, so eventually someone<br>
> else (Sebastian, I think) had to debug why those changes didn't work<br>
> right. So, if the 64-bit cleaning changes aren't tested, why they<br>
> weren't tested must be discussed, and notes should be left behind in<br>
> case someone later tries to run the code and it doesn't work as<br>
> expected. Much better would be to test the changes if it is at all<br>
> possible.<br>
><br>
> Gedare<br>
</blockquote></div>
</blockquote></div></div>