[PATCH v3 1/3] rtems-debugger: Fixed 32bit pointers
Joel Sherrill
joel at rtems.org
Mon Mar 22 17:58:06 UTC 2021
On Mon, Mar 22, 2021 at 12:54 PM Joel Sherrill <joel at 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 <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/c2f18f81/attachment.html>
More information about the devel
mailing list