[PATCH v2 1/3] rtems-debugger: Fixed 32bit pointers

[Gedare Bloom]

On Thu, Mar 18, 2021 at 12:14 PM Joel Sherrill <joel at rtems.org> wrote:
>
>
>
> On Thu, Mar 18, 2021, 1:10 PM Stephen Clark <stephen.clark at oarcorp.com> wrote:
>>
>> I think Sebastian and Gedare might have referenced this earlier, but I’m not sure if how to be that descriptive within the 50 character limit. "Use uintptr_t not uint32_t for portability to 64-bit CPUs" is 58 characters without a prefix. Even when pared down to something like “Use uintptr_t to build on 64-bit CPUs”, there still isn’t room to prepend “rtems-debugger:”.
>
>
> I don't think these HAVE to be less than 80 columns just close. But you could drop "not uint32_t" and "for portability" if needed


In the first line of the commit, we want to adhere to about 50
characters. Otherwise, the short-log and email subject lines get
excessive.

I think the commit message he used is fine, it gives sufficient
information about *what* has been fixed *where*, while the details can
be gotten by drilling down another level.

Brevity in the first line is appreciated. I'm sure we had written this
down somewhere...

>>
>>
>>
>> From: Joel Sherrill <joel at rtems.org>
>> Sent: Thursday, March 18, 2021 12:50 PM
>> To: Stephen Clark <stephen.clark at oarcorp.com>
>> Cc: rtems-devel at rtems.org <devel at rtems.org>
>> Subject: Re: [PATCH v2 1/3] rtems-debugger: Fixed 32bit pointers
>>
>>
>>
>> After picking on Ryan, Alex, and Sebastian, you get it next. :)
>>
>>
>>
>> "Fix" or "Fixed" in the short commit message title is not useful
>>
>> when you browse the log of commits:
>>
>> https://git.rtems.org/rtems/log/
>>
>> Something like "Use uint32_t not uintptr_t for portability to 64-bit CPUs"
>>
>> says a lot more. Think of yourself reading that list of commits and what
>>
>> messages tell you enough and which leave you thinking that you need
>>
>> to look at the change to know what was done.
>>
>>
>>
>>
>>
>>
>>
>> On Thu, Mar 18, 2021 at 12:33 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.
>> ---
>>  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]);
>> @@ -1468,7 +1468,7 @@ 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]);
>>
>>
>>
>> I think the changes OK but Chris should comment what
>>
>> happens on 64-bit address targets.
>>
>>
>>
>> I think this may be decoding the gdb protocol message and we need
>>
>> to know if the field coming in is OK to decode as a uint.
>>
>>
>>
>> Your patch is OK but there may be an issue interfacing with the protocol
>>
>> that this points out.
>>
>>
>>
>> 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)
>>  {
>>    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,
>>                                                   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