[PATCH v4] rtems-debugger: Fixed 32bit pointers
Gedare Bloom
gedare at rtems.org
Thu May 6 17:25:39 UTC 2021
On Thu, May 6, 2021 at 10:13 AM Joel Sherrill <joel at rtems.org> wrote:
>
>
>
> On Wed, May 5, 2021 at 12:35 AM Gedare Bloom <gedare at rtems.org> wrote:
>>
>> On Tue, May 4, 2021 at 1:34 PM Stephen Clark <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. Added hex_decode_addr
>> > function to rtems-debugger.
>> > ---
>> > .../rtems/debugger/rtems-debugger-server.h | 5 ++++
>> > cpukit/libdebugger/rtems-debugger-server.c | 30 +++++++++++++++----
>> > cpukit/libdebugger/rtems-debugger-target.c | 2 +-
>> > cpukit/libdebugger/rtems-debugger-target.h | 2 +-
>> > 4 files changed, 32 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/cpukit/include/rtems/debugger/rtems-debugger-server.h b/cpukit/include/rtems/debugger/rtems-debugger-server.h
>> > index 2189aac873..beff302641 100644
>> > --- a/cpukit/include/rtems/debugger/rtems-debugger-server.h
>> > +++ b/cpukit/include/rtems/debugger/rtems-debugger-server.h
>> > @@ -50,6 +50,11 @@ extern "C" {
>> > */
>> > #define DB_UINT uint32_t
>> >
>> > +/**
>> > + * Data type for addresses. Here for 64bit support.
>> > + */
>> > +#define DB_ADDR uintptr_t
>> > +
>> > /*
>> > * Debugger signals.
>> > */
>> > diff --git a/cpukit/libdebugger/rtems-debugger-server.c b/cpukit/libdebugger/rtems-debugger-server.c
>> > index 975ec23a30..2ada418a79 100644
>> > --- a/cpukit/libdebugger/rtems-debugger-server.c
>> > +++ b/cpukit/libdebugger/rtems-debugger-server.c
>> > @@ -154,6 +154,26 @@ hex_encode(int val)
>> > return "0123456789abcdef"[val & 0xf];
>> > }
>> >
>> > +static inline DB_ADDR
>> > +hex_decode_addr(const uint8_t* data)
>> > +{
>> > + DB_ADDR ui = 0;
>> > + size_t i;
>> > + if (data[0] == '-') {
>> > + if (data[1] == '1')
>> > + ui = (DB_ADDR) -1;
>> > + }
>> > + else {
>> > + for (i = 0; i < (sizeof(ui) * 2); ++i) {
>> > + int v = hex_decode(data[i]);
>> > + if (v < 0)
>> > + break;
>> > + ui = (ui << 4) | v;
>> > + }
>> > + }
>> > + return ui;
>> > +}
>> > +
>> This function body is identical to the following hex_decode_uint()
>> except for the type of DB_ADDR. They could probably be merged. Is
>> there a reason not to combine them and avoid the copy-paste?
>
>
> DB_ADDR aka uintptr_t is just stated as being large enough to hold
> an address. It is not listed in the set of types the C Standard makes
> size guarantees about. See
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf in
> section 5.2.4.2.1 for the relationship between types that are guaranteed.
> Notice that uintptr_t is just described in English when it shows up
> in the standard and not in this list.
>
> I don't think we can assume any specific integer type is equivalent to
> uintptr_t.
>
> It is a good example for function templates but they are not in C.
>
OK. probably this can be done with macros, but I won't push for that :)
>
>>
>> > static inline DB_UINT
>> > hex_decode_uint(const uint8_t* data)
>> > {
>> > @@ -1438,10 +1458,10 @@ 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]);
>> > + addr = hex_decode_addr(&buffer[1]);
>> > length = hex_decode_uint((const uint8_t*) comma + 1);
>> > remote_packet_out_reset();
>> > r = rtems_debugger_target_start_memory_access();
>> > @@ -1468,10 +1488,10 @@ remote_write_memory(uint8_t* buffer, int size)
>> > comma = strchr((const char*) buffer, ',');
>> > colon = strchr((const char*) buffer, ':');
>> > if (comma != NULL && colon != NULL) {
>> > - DB_UINT addr;
>> > + uintptr_t addr;
>> > DB_UINT length;
>> > int r;
>> > - addr = hex_decode_uint(&buffer[1]);
>> > + addr = hex_decode_addr(&buffer[1]);
>> > length = hex_decode_uint((const uint8_t*) comma + 1);
>> > r = rtems_debugger_target_start_memory_access();
>> > if (r == 0) {
>> > @@ -1521,7 +1541,7 @@ remote_breakpoints(bool insert, uint8_t* buffer, int size)
>> > uint32_t capabilities;
>> > DB_UINT addr;
>> > DB_UINT kind;
>> > - addr = hex_decode_uint((const uint8_t*) comma1 + 1);
>> > + addr = hex_decode_addr((const uint8_t*) comma1 + 1);
>> > kind = hex_decode_uint((const uint8_t*)comma2 + 1);
>> > capabilities = rtems_debugger_target_capabilities();
>> > switch (buffer[1]) {
>> > diff --git a/cpukit/libdebugger/rtems-debugger-target.c b/cpukit/libdebugger/rtems-debugger-target.c
>> > index bf7579700d..34e4e84d2f 100644
>> > --- a/cpukit/libdebugger/rtems-debugger-target.c
>> > +++ b/cpukit/libdebugger/rtems-debugger-target.c
>> > @@ -168,7 +168,7 @@ rtems_debugger_target_reg_table_size(void)
>> > }
>> >
>> > int
>> > -rtems_debugger_target_swbreak_control(bool insert, DB_UINT addr, DB_UINT kind)
>> > +rtems_debugger_target_swbreak_control(bool insert, uintptr_t addr, DB_UINT kind)
>> why not use DB_ADDR?
>
>
> That should be DB_ADDR unless there is another sweep to eliminate DB_ADDR.
> But I wouldn't do that on this pass.
>
I'm confused. You're adding DB_ADDR, why would you eliminate it? This
patch adds DB_ADDR, it makes sense to also have it replacing DB_UINT
with DB_ADDR in cases where addresses are being used. We should use
the right type names. Please make these DB_UINT -> DB_ADDR conversions
for consistent type use.
>>
>>
>> > {
>> > rtems_debugger_target* target = rtems_debugger->target;
>> > rtems_debugger_target_swbreak* swbreaks;
>> > diff --git a/cpukit/libdebugger/rtems-debugger-target.h b/cpukit/libdebugger/rtems-debugger-target.h
>> > index f2abbe5fd3..db356e1f07 100644
>> > --- a/cpukit/libdebugger/rtems-debugger-target.h
>> > +++ b/cpukit/libdebugger/rtems-debugger-target.h
>> > @@ -200,7 +200,7 @@ extern void rtems_debugger_target_exception_print(CPU_Exception_frame* frame);
>> > * Software breakpoints. These are also referred to as memory breakpoints.
>> > */
>> > extern int rtems_debugger_target_swbreak_control(bool insert,
>> > - DB_UINT addr,
>> > + uintptr_t addr,
>> ditto
>>
>> > DB_UINT kind);
>> >
>> > /**
>> > --
>> > 2.27.0
>> >
>> > _______________________________________________
>> > devel mailing list
>> > devel at rtems.org
>> > http://lists.rtems.org/mailman/listinfo/devel
>> _______________________________________________
>> devel mailing list
>> devel at rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list