[1/2] gdbserver: aarch64: Fix expedited registers list

Message ID 20240831005607.2478217-1-thiago.bauermann@linaro.org
State New
Headers
Series [1/2] gdbserver: aarch64: Fix expedited registers list |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Thiago Jung Bauermann Aug. 31, 2024, 12:56 a.m. UTC
  Since this commit:

  commit a8651ef51822f91ec86d0d5caffbf2e50b174c23
  CommitDate: Fri Jun 14 14:47:38 2024 +0100

      gdb/aarch64: prevent crash from in process agent

gdbserver isn't sending expedited registers with its stop reply packets
anymore.  The problem is with how the constructor of the
expedited_registers std::vector is called:

The intent of the expedited_registers initialization in
aarch64_linux_read_description is to create a vector with capacity for 6
elements, but that's not how the std::vector constructor works.

Instead it creates a vector pre-populated with 6 elements initialized
with the default value for the type of the elements, and thus the first
6 elements are null pointers.  The actual expedited registers are added
starting at the 7th element.

This causes init_target_desc to consider that the expedite_regs list is
empty, since it stops checking at the first nullptr element.  The end
result is that gdbserver doesn't send any expedited registers to GDB in
its stop replies.

Fix by not specifying an element count when declaring the vector.

Tested for regressions on aarch64-linux-gnu native-extended-remote.
---
 gdbserver/linux-aarch64-tdesc.cc | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)


base-commit: a253bea8995323201b016fe477280c1782688ab4
  

Comments

Andrew Burgess Sept. 4, 2024, 2:40 p.m. UTC | #1
Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:

> Since this commit:
>
>   commit a8651ef51822f91ec86d0d5caffbf2e50b174c23
>   CommitDate: Fri Jun 14 14:47:38 2024 +0100
>
>       gdb/aarch64: prevent crash from in process agent
>
> gdbserver isn't sending expedited registers with its stop reply packets
> anymore.  The problem is with how the constructor of the
> expedited_registers std::vector is called:
>
> The intent of the expedited_registers initialization in
> aarch64_linux_read_description is to create a vector with capacity for 6
> elements, but that's not how the std::vector constructor works.
>
> Instead it creates a vector pre-populated with 6 elements initialized
> with the default value for the type of the elements, and thus the first
> 6 elements are null pointers.  The actual expedited registers are added
> starting at the 7th element.
>
> This causes init_target_desc to consider that the expedite_regs list is
> empty, since it stops checking at the first nullptr element.  The end
> result is that gdbserver doesn't send any expedited registers to GDB in
> its stop replies.
>
> Fix by not specifying an element count when declaring the vector.

Oops.  Thanks for fixing this.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>
> Tested for regressions on aarch64-linux-gnu native-extended-remote.
> ---
>  gdbserver/linux-aarch64-tdesc.cc | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
> index 5d3b6ddffffa..31ec7854cc0b 100644
> --- a/gdbserver/linux-aarch64-tdesc.cc
> +++ b/gdbserver/linux-aarch64-tdesc.cc
> @@ -52,14 +52,10 @@ aarch64_linux_read_description (const aarch64_features &features)
>      {
>        tdesc = aarch64_create_target_description (features);
>  
> -      /* Configure the expedited registers.  By default we include x29, sp
> -	 and pc, but we allow for up to 6 pointers as this is (currently)
> -	 the most that we push.
> -
> -	 Calling init_target_desc takes a copy of all the strings pointed
> -	 to by expedited_registers so this vector only needs to live for
> -	 the scope of this function.  */
> -      std::vector<const char *> expedited_registers (6);
> +      /* Configure the expedited registers.  Calling init_target_desc takes
> +	 a copy of all the strings pointed to by expedited_registers so this
> +	 vector only needs to live for the scope of this function.  */
> +      std::vector<const char *> expedited_registers;
>        expedited_registers.push_back ("x29");
>        expedited_registers.push_back ("sp");
>        expedited_registers.push_back ("pc");
>
> base-commit: a253bea8995323201b016fe477280c1782688ab4
  
Thiago Jung Bauermann Sept. 5, 2024, 4:10 a.m. UTC | #2
Hello Andrew,

Andrew Burgess <aburgess@redhat.com> writes:

> Thiago Jung Bauermann <thiago.bauermann@linaro.org> writes:
>
>> Since this commit:
>>
>>   commit a8651ef51822f91ec86d0d5caffbf2e50b174c23
>>   CommitDate: Fri Jun 14 14:47:38 2024 +0100
>>
>>       gdb/aarch64: prevent crash from in process agent
>>
>> gdbserver isn't sending expedited registers with its stop reply packets
>> anymore.  The problem is with how the constructor of the
>> expedited_registers std::vector is called:
>>
>> The intent of the expedited_registers initialization in
>> aarch64_linux_read_description is to create a vector with capacity for 6
>> elements, but that's not how the std::vector constructor works.
>>
>> Instead it creates a vector pre-populated with 6 elements initialized
>> with the default value for the type of the elements, and thus the first
>> 6 elements are null pointers.  The actual expedited registers are added
>> starting at the 7th element.
>>
>> This causes init_target_desc to consider that the expedite_regs list is
>> empty, since it stops checking at the first nullptr element.  The end
>> result is that gdbserver doesn't send any expedited registers to GDB in
>> its stop replies.
>>
>> Fix by not specifying an element count when declaring the vector.
>
> Oops.  Thanks for fixing this.
>
> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thank you! Pushed as commit 43af2e08dc0a.

As an aside, C++26 will include std::inplace_vector which works that way.
  

Patch

diff --git a/gdbserver/linux-aarch64-tdesc.cc b/gdbserver/linux-aarch64-tdesc.cc
index 5d3b6ddffffa..31ec7854cc0b 100644
--- a/gdbserver/linux-aarch64-tdesc.cc
+++ b/gdbserver/linux-aarch64-tdesc.cc
@@ -52,14 +52,10 @@  aarch64_linux_read_description (const aarch64_features &features)
     {
       tdesc = aarch64_create_target_description (features);
 
-      /* Configure the expedited registers.  By default we include x29, sp
-	 and pc, but we allow for up to 6 pointers as this is (currently)
-	 the most that we push.
-
-	 Calling init_target_desc takes a copy of all the strings pointed
-	 to by expedited_registers so this vector only needs to live for
-	 the scope of this function.  */
-      std::vector<const char *> expedited_registers (6);
+      /* Configure the expedited registers.  Calling init_target_desc takes
+	 a copy of all the strings pointed to by expedited_registers so this
+	 vector only needs to live for the scope of this function.  */
+      std::vector<const char *> expedited_registers;
       expedited_registers.push_back ("x29");
       expedited_registers.push_back ("sp");
       expedited_registers.push_back ("pc");