[PATCH v3 1/3] rtems-debugger: Fixed 32bit pointers
Joel Sherrill
joel at rtems.org
Mon Mar 22 17:54:06 UTC 2021
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.
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 <gedare at rtems.org> wrote:
> On Mon, Mar 22, 2021 at 10:44 AM Gedare Bloom <gedare at rtems.org> wrote:
> >
> > On Sun, Mar 21, 2021 at 6:46 PM Chris Johns <chrisj at rtems.org> wrote:
> > >
> > >
> > >
> > > On 21/3/21 2:26 am, Joel Sherrill wrote:
> > > >
> > > >
> > > > On Sat, Mar 20, 2021, 1:21 AM Gedare Bloom <gedare at rtems.org
> > > > <mailto:gedare at rtems.org>> wrote:
> > > >
> > > > Hi Stephen, Joel:
> > > >
> > > > On Fri, Mar 19, 2021 at 9:35 AM Stephen Clark <
> stephen.clark at oarcorp.com
> > > > <mailto:stephen.clark at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20210322/1817c1f2/attachment-0001.html>
More information about the devel
mailing list