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

2021-03-18 Thread Chris Johns 
On 19/3/21 4:49 am, Joel Sherrill wrote:
> On Thu, Mar 18, 2021 at 12:33 PM Stephen Clark  > 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([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([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.

The change looks fine to me

Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

2021-03-18 Thread Gedare Bloom 
On Thu, Mar 18, 2021 at 12:14 PM Joel Sherrill  wrote:
>
>
>
> On Thu, Mar 18, 2021, 1:10 PM Stephen Clark  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 
>> Sent: Thursday, March 18, 2021 12:50 PM
>> To: Stephen Clark 
>> Cc: rtems-de...@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  
>> 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([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([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

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

2021-03-18 Thread Joel Sherrill 
On Thu, Mar 18, 2021, 1:10 PM Stephen Clark 
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

>
>
> *From:* Joel Sherrill 
> *Sent:* Thursday, March 18, 2021 12:50 PM
> *To:* Stephen Clark 
> *Cc:* rtems-de...@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 
> 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([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([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(boolinsert,
> - DB_UINT addr,
> + uintptr_t addr,
>   DB_UINT kind);
>
>  /**
> --
> 2.27.0
>
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>
>
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

2021-03-18 Thread Stephen Clark 
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:”.

From: Joel Sherrill 
Sent: Thursday, March 18, 2021 12:50 PM
To: Stephen Clark 
Cc: rtems-de...@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 
mailto:stephen.cl...@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([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([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(boolinsert,
- DB_UINT addr,
+ uintptr_t addr,
  DB_UINT kind);

 /**
--
2.27.0

___
devel mailing list
devel@rtems.org<mailto:devel@rtems.org>
http://lists.rtems.org/mailman/listinfo/devel
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

2021-03-18 Thread Joel Sherrill 
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 
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([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([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(boolinsert,
> - DB_UINT addr,
> + uintptr_t addr,
>   DB_UINT kind);
>
>  /**
> --
> 2.27.0
>
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

2021-03-18 Thread Stephen Clark 
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([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([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)
 {
   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(boolinsert,
- DB_UINT addr,
+ uintptr_t addr,
  DB_UINT kind);
 
 /**
-- 
2.27.0

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel