[AArch64] Fix PR gdb/28681

Message ID 20220104172254.3665546-1-luis.machado@linaro.org
State New
Headers show
Series
  • [AArch64] Fix PR gdb/28681
Related show

Commit Message

Simon Marchi via Gdb-patches Jan. 4, 2022, 5:22 p.m.
This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but
fixing things for AArch64.

With the patch, gdb.cp/non-trivial-retval.exp has full passes on
AArch64-Linux Ubuntu 20.04/18.04.
---
 gdb/aarch64-tdep.c | 9 +++++++++
 1 file changed, 9 insertions(+)

-- 
2.25.1

Comments

Simon Marchi via Gdb-patches Jan. 4, 2022, 6:09 p.m. | #1
On 2022-01-04 12:22, Luis Machado via Gdb-patches wrote:
> This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but

> fixing things for AArch64.

> 

> With the patch, gdb.cp/non-trivial-retval.exp has full passes on

> AArch64-Linux Ubuntu 20.04/18.04.

> ---

>  gdb/aarch64-tdep.c | 9 +++++++++

>  1 file changed, 9 insertions(+)

> 

> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c

> index 70fb66954a4..802762f303c 100644

> --- a/gdb/aarch64-tdep.c

> +++ b/gdb/aarch64-tdep.c

> @@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,

>  	  valbuf += X_REGISTER_SIZE;

>  	}

>      }

> +  else if (!language_pass_by_reference (type).trivially_copyable)

> +    {

> +      /* If the object is a non-trivial C++ object, the result is passed as a

> +	 pointer stored in X0.  */

> +      CORE_ADDR addr;

> +

> +      regs->cooked_read (AARCH64_X0_REGNUM, &addr);

> +      read_memory (addr, valbuf, TYPE_LENGTH (type));

> +    }

>    else

>      {

>        /* For a structure or union the behaviour is as if the value had


I'll let somebody else review this (probably Andrew), but please change the
patch subject to something descriptive, not just the bug number.

Thanks,

Simon
Simon Marchi via Gdb-patches Jan. 4, 2022, 6:44 p.m. | #2
On 1/4/22 3:09 PM, Simon Marchi wrote:
> 

> 

> On 2022-01-04 12:22, Luis Machado via Gdb-patches wrote:

>> This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but

>> fixing things for AArch64.

>>

>> With the patch, gdb.cp/non-trivial-retval.exp has full passes on

>> AArch64-Linux Ubuntu 20.04/18.04.

>> ---

>>   gdb/aarch64-tdep.c | 9 +++++++++

>>   1 file changed, 9 insertions(+)

>>

>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c

>> index 70fb66954a4..802762f303c 100644

>> --- a/gdb/aarch64-tdep.c

>> +++ b/gdb/aarch64-tdep.c

>> @@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,

>>   	  valbuf += X_REGISTER_SIZE;

>>   	}

>>       }

>> +  else if (!language_pass_by_reference (type).trivially_copyable)

>> +    {

>> +      /* If the object is a non-trivial C++ object, the result is passed as a

>> +	 pointer stored in X0.  */

>> +      CORE_ADDR addr;

>> +

>> +      regs->cooked_read (AARCH64_X0_REGNUM, &addr);

>> +      read_memory (addr, valbuf, TYPE_LENGTH (type));

>> +    }

>>     else

>>       {

>>         /* For a structure or union the behaviour is as if the value had

> 

> I'll let somebody else review this (probably Andrew), but please change the

> patch subject to something descriptive, not just the bug number.


Did you want the entire bugzilla PR subject or something else? I can't 
really tell from your reply, sorry.
Simon Marchi via Gdb-patches Jan. 4, 2022, 6:47 p.m. | #3
On 2022-01-04 13:44, Luis Machado wrote:
> On 1/4/22 3:09 PM, Simon Marchi wrote:

>>

>>

>> On 2022-01-04 12:22, Luis Machado via Gdb-patches wrote:

>>> This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but

>>> fixing things for AArch64.

>>>

>>> With the patch, gdb.cp/non-trivial-retval.exp has full passes on

>>> AArch64-Linux Ubuntu 20.04/18.04.

>>> ---

>>>   gdb/aarch64-tdep.c | 9 +++++++++

>>>   1 file changed, 9 insertions(+)

>>>

>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c

>>> index 70fb66954a4..802762f303c 100644

>>> --- a/gdb/aarch64-tdep.c

>>> +++ b/gdb/aarch64-tdep.c

>>> @@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,

>>>         valbuf += X_REGISTER_SIZE;

>>>       }

>>>       }

>>> +  else if (!language_pass_by_reference (type).trivially_copyable)

>>> +    {

>>> +      /* If the object is a non-trivial C++ object, the result is passed as a

>>> +     pointer stored in X0.  */

>>> +      CORE_ADDR addr;

>>> +

>>> +      regs->cooked_read (AARCH64_X0_REGNUM, &addr);

>>> +      read_memory (addr, valbuf, TYPE_LENGTH (type));

>>> +    }

>>>     else

>>>       {

>>>         /* For a structure or union the behaviour is as if the value had

>>

>> I'll let somebody else review this (probably Andrew), but please change the

>> patch subject to something descriptive, not just the bug number.

> 

> Did you want the entire bugzilla PR subject or something else? I can't really tell from your reply, sorry.


No, not the bugzilla title (which is `Wrong pretty-printed unique_ptr
value when using "finish"`), since that is a distant symptom of the
problem you fix.  The subject should state what you are fixing, so
something about the handling of trivially copyable return values on
AArch64 (something like that, I didn't follow the resolution of the bug
close enough).

Simon
Simon Marchi via Gdb-patches Jan. 4, 2022, 6:49 p.m. | #4
On 1/4/22 3:47 PM, Simon Marchi wrote:
> 

> 

> On 2022-01-04 13:44, Luis Machado wrote:

>> On 1/4/22 3:09 PM, Simon Marchi wrote:

>>>

>>>

>>> On 2022-01-04 12:22, Luis Machado via Gdb-patches wrote:

>>>> This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but

>>>> fixing things for AArch64.

>>>>

>>>> With the patch, gdb.cp/non-trivial-retval.exp has full passes on

>>>> AArch64-Linux Ubuntu 20.04/18.04.

>>>> ---

>>>>    gdb/aarch64-tdep.c | 9 +++++++++

>>>>    1 file changed, 9 insertions(+)

>>>>

>>>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c

>>>> index 70fb66954a4..802762f303c 100644

>>>> --- a/gdb/aarch64-tdep.c

>>>> +++ b/gdb/aarch64-tdep.c

>>>> @@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,

>>>>          valbuf += X_REGISTER_SIZE;

>>>>        }

>>>>        }

>>>> +  else if (!language_pass_by_reference (type).trivially_copyable)

>>>> +    {

>>>> +      /* If the object is a non-trivial C++ object, the result is passed as a

>>>> +     pointer stored in X0.  */

>>>> +      CORE_ADDR addr;

>>>> +

>>>> +      regs->cooked_read (AARCH64_X0_REGNUM, &addr);

>>>> +      read_memory (addr, valbuf, TYPE_LENGTH (type));

>>>> +    }

>>>>      else

>>>>        {

>>>>          /* For a structure or union the behaviour is as if the value had

>>>

>>> I'll let somebody else review this (probably Andrew), but please change the

>>> patch subject to something descriptive, not just the bug number.

>>

>> Did you want the entire bugzilla PR subject or something else? I can't really tell from your reply, sorry.

> 

> No, not the bugzilla title (which is `Wrong pretty-printed unique_ptr

> value when using "finish"`), since that is a distant symptom of the

> problem you fix.  The subject should state what you are fixing, so

> something about the handling of trivially copyable return values on

> AArch64 (something like that, I didn't follow the resolution of the bug

> close enough).


How about "gdb: on aarch64 non-trivial C++ objects are returned in memory"?
Simon Marchi via Gdb-patches Jan. 4, 2022, 6:56 p.m. | #5
> How about "gdb: on aarch64 non-trivial C++ objects are returned in memory"?


Sounds good.  Although if I had to be picky, I'd say it's not perfect
because it just states a fact (as in "the sky is blue"), and doesn't
describe the code change the patch does.  I would go with something like
"gdb/aarch64: fix return value location of non-trivial C++ objects".
The commit message would then say that the architecture's ABI describes
that this kind of object must be returned in memory, but GDB currently
erroneously returns it in registers, and that the patch fixes that.

Simon
Simon Marchi via Gdb-patches Jan. 4, 2022, 7:04 p.m. | #6
cc-ing Alan back, since the mailing list software seems to have removed 
it, although I see it on my end.

On 1/4/22 3:56 PM, Simon Marchi wrote:
>> How about "gdb: on aarch64 non-trivial C++ objects are returned in memory"?

> 

> Sounds good.  Although if I had to be picky, I'd say it's not perfect

> because it just states a fact (as in "the sky is blue"), and doesn't

> describe the code change the patch does.  I would go with something like

> "gdb/aarch64: fix return value location of non-trivial C++ objects".

> The commit message would then say that the architecture's ABI describes

> that this kind of object must be returned in memory, but GDB currently

> erroneously returns it in registers, and that the patch fixes that.


Fair enough.
Simon Marchi via Gdb-patches Jan. 5, 2022, 12:54 p.m. | #7
* Luis Machado <luis.machado@linaro.org> [2022-01-04 14:22:54 -0300]:

> This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but

> fixing things for AArch64.

> 

> With the patch, gdb.cp/non-trivial-retval.exp has full passes on

> AArch64-Linux Ubuntu 20.04/18.04.

> ---

>  gdb/aarch64-tdep.c | 9 +++++++++

>  1 file changed, 9 insertions(+)

> 

> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c

> index 70fb66954a4..802762f303c 100644

> --- a/gdb/aarch64-tdep.c

> +++ b/gdb/aarch64-tdep.c

> @@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,

>  	  valbuf += X_REGISTER_SIZE;

>  	}

>      }

> +  else if (!language_pass_by_reference (type).trivially_copyable)

> +    {

> +      /* If the object is a non-trivial C++ object, the result is passed as a

> +	 pointer stored in X0.  */

> +      CORE_ADDR addr;

> +

> +      regs->cooked_read (AARCH64_X0_REGNUM, &addr);

> +      read_memory (addr, valbuf, TYPE_LENGTH (type));

> +    }

>    else

>      {

>        /* For a structure or union the behaviour is as if the value had


The aarch64_extract_return_value function is called exclusively from
aarch64_return_value.

After calling this function the aarch64_return_value function returns
RETURN_VALUE_REGISTER_CONVENTION, which I don't think is correct in
the above case.

I think we should be returning one of either
RETURN_VALUE_STRUCT_CONVENTION, RETURN_VALUE_ABI_RETURNS_ADDRESS or
RETURN_VALUE_ABI_PRESERVES_ADDRESS.

I wonder if aarch64_return_in_memory should be doing more of the work
in this case?  But that's just a thought, I'm sure whatever you come
up with will be fine, so long as the return type is correct.

Thanks,
Andrew
Simon Marchi via Gdb-patches Jan. 11, 2022, 9:17 p.m. | #8
On 1/5/22 9:54 AM, Andrew Burgess wrote:
> * Luis Machado <luis.machado@linaro.org> [2022-01-04 14:22:54 -0300]:

> 

>> This is the same as commit b1718fcdd1d2a5c514f8ee504ba07fb3f42b8608, but

>> fixing things for AArch64.

>>

>> With the patch, gdb.cp/non-trivial-retval.exp has full passes on

>> AArch64-Linux Ubuntu 20.04/18.04.

>> ---

>>   gdb/aarch64-tdep.c | 9 +++++++++

>>   1 file changed, 9 insertions(+)

>>

>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c

>> index 70fb66954a4..802762f303c 100644

>> --- a/gdb/aarch64-tdep.c

>> +++ b/gdb/aarch64-tdep.c

>> @@ -2323,6 +2323,15 @@ aarch64_extract_return_value (struct type *type, struct regcache *regs,

>>   	  valbuf += X_REGISTER_SIZE;

>>   	}

>>       }

>> +  else if (!language_pass_by_reference (type).trivially_copyable)

>> +    {

>> +      /* If the object is a non-trivial C++ object, the result is passed as a

>> +	 pointer stored in X0.  */

>> +      CORE_ADDR addr;

>> +

>> +      regs->cooked_read (AARCH64_X0_REGNUM, &addr);

>> +      read_memory (addr, valbuf, TYPE_LENGTH (type));

>> +    }

>>     else

>>       {

>>         /* For a structure or union the behaviour is as if the value had

> 

> The aarch64_extract_return_value function is called exclusively from

> aarch64_return_value.

> 

> After calling this function the aarch64_return_value function returns

> RETURN_VALUE_REGISTER_CONVENTION, which I don't think is correct in

> the above case.

> 

> I think we should be returning one of either

> RETURN_VALUE_STRUCT_CONVENTION, RETURN_VALUE_ABI_RETURNS_ADDRESS or

> RETURN_VALUE_ABI_PRESERVES_ADDRESS.


Yes, it seems that way.

The original return convention does not cover this particular case. The 
current code may actually be incorrect, and 
RETURN_VALUE_ABI_RETURNS_ADDRESS seems to be a more correct option.

> 

> I wonder if aarch64_return_in_memory should be doing more of the work

> in this case?  But that's just a thought, I'm sure whatever you come

> up with will be fine, so long as the return type is correct.


Expanding aarch64_return_in_memory to check for non-trivially-copyable 
objects and filling in the readbuf with the right value should do it as 
well.

I have an updated patch for this.

Thanks,
Luis

Patch

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 70fb66954a4..802762f303c 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2323,6 +2323,15 @@  aarch64_extract_return_value (struct type *type, struct regcache *regs,
 	  valbuf += X_REGISTER_SIZE;
 	}
     }
+  else if (!language_pass_by_reference (type).trivially_copyable)
+    {
+      /* If the object is a non-trivial C++ object, the result is passed as a
+	 pointer stored in X0.  */
+      CORE_ADDR addr;
+
+      regs->cooked_read (AARCH64_X0_REGNUM, &addr);
+      read_memory (addr, valbuf, TYPE_LENGTH (type));
+    }
   else
     {
       /* For a structure or union the behaviour is as if the value had