[PR,gdb/22736,aarch64] gdb crashes on a conditional breakpoint with cast return type

  Don't call language_pass_by_reference() with function that has no return type.

Only call language_pass_by_reference(), which returns whether or not an 
additional initial argument has been given, when return_type is not NULL
in function aarch64_push_dummy_call().

Tested on aarch64-linux-gnu. No regressions.
---
 gdb/ChangeLog      | 6 ++++++
 gdb/aarch64-tdep.c | 4 +++-
 2 files changed, 9 insertions(+), 1 deletion(-)
  

On 2018-05-25 19:20, Weimin Pan wrote:
> Don't call language_pass_by_reference() with function that has no 
> return type.
> 
> Only call language_pass_by_reference(), which returns whether or not an
> additional initial argument has been given, when return_type is not 
> NULL
> in function aarch64_push_dummy_call().

Hi Weimin,

Since Pedro's patch that makes GDB not assume that the return type of 
functions without debug info is int:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=7022349d5c86bae74b49225515f42d2e221bd368

I think we will always know the return type of the function.  Either 
it's in the debug info or it's provided by the user.  In 
call_function_by_hand_dummy, if the debug info doesn't provide the 
return type of the function, we use the type of the user-provided cast:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/infcall.c;h=cd3eedfeeb712b27234a68cf8af394558ce4f57d;hb=cd3eedfeeb712b27234a68cf8af394558ce4f57d#l870

I think the default_return_type could be passed down to 
gdbarch_push_dummy_call and used the same way, so that we always have a 
return type.

Also, could you add a test case for this?  I was able to create a simple 
C++ (not C) program made from an object file built with no debug info:

int returns_two ()
{
   return 2;
}

and one built with debug info:

int returns_two();

void func()
{
}

int main()
{
   func();
   return 0;
}


Putting this breakpoint and running crashes GDB:

(gdb) b func if (int)returns_two() == 2"

Thanks,

Simon
  

On 5/25/2018 6:14 PM, Simon Marchi wrote:
> On 2018-05-25 19:20, Weimin Pan wrote:
>> Don't call language_pass_by_reference() with function that has no 
>> return type.
>>
>> Only call language_pass_by_reference(), which returns whether or not an
>> additional initial argument has been given, when return_type is not NULL
>> in function aarch64_push_dummy_call().
>
> Hi Weimin,
>
> Since Pedro's patch that makes GDB not assume that the return type of 
> functions without debug info is int:
>
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=7022349d5c86bae74b49225515f42d2e221bd368 
>
>
> I think we will always know the return type of the function. Either 
> it's in the debug info or it's provided by the user.  In 
> call_function_by_hand_dummy, if the debug info doesn't provide the 
> return type of the function, we use the type of the user-provided cast:
>
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/infcall.c;h=cd3eedfeeb712b27234a68cf8af394558ce4f57d;hb=cd3eedfeeb712b27234a68cf8af394558ce4f57d#l870 
>
>
> I think the default_return_type could be passed down to 
> gdbarch_push_dummy_call and used the same way, so that we always have 
> a return type.

Hi Simon,

Since call_function_by_hand_dummy () already calls 
gdbarch_return_in_first_hidden_param_p() and sets
hidden_first_param_p accordingly. Instead of passing the 
deault_return_type and having the target make
the same call again , I think we should just pass hidden_first_param_p 
to gdbarch_push_dummy_call()?

>
> Also, could you add a test case for this?  I was able to create a 
> simple C++ (not C) program made from an object file built with no 
> debug info:
>
> int returns_two ()
> {
>   return 2;
> }
>
> and one built with debug info:
>
> int returns_two();
>
> void func()
> {
> }
>
> int main()
> {
>   func();
>   return 0;
> }
>
>
> Putting this breakpoint and running crashes GDB:
>
> (gdb) b func if (int)returns_two() == 2"

Yes, will do and maybe use the one you provided here.

Thanks for your comments.

Weimin

>
> Thanks,
>
> Simon
  

On 05/26/2018 02:14 AM, Simon Marchi wrote:
> On 2018-05-25 19:20, Weimin Pan wrote:
>> Don't call language_pass_by_reference() with function that has no return type.
>>
>> Only call language_pass_by_reference(), which returns whether or not an
>> additional initial argument has been given, when return_type is not NULL
>> in function aarch64_push_dummy_call().
> 
> Hi Weimin,
> 
> Since Pedro's patch that makes GDB not assume that the return type of functions without debug info is int:
> 
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=7022349d5c86bae74b49225515f42d2e221bd368
> 
> I think we will always know the return type of the function.  Either it's in the debug info or it's provided by the user.  In call_function_by_hand_dummy, if the debug info doesn't provide the return type of the function, we use the type of the user-provided cast:
> 
> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/infcall.c;h=cd3eedfeeb712b27234a68cf8af394558ce4f57d;hb=cd3eedfeeb712b27234a68cf8af394558ce4f57d#l870
> 
> I think the default_return_type could be passed down to gdbarch_push_dummy_call and used the same way, so that we always have a return type.

Agreed.  

Note this bug discussed earlier, and Alan had a patch too:

 https://sourceware.org/ml/gdb-patches/2018-03/msg00157.html

That was discussed just before the recent ifunc revamp, and I
wasn't exactly sure whether master still had the issue.  Also I
forgot about it.  :-P

Alan, do you recall the status of that from your end?

The issue of using the cast-to type was discussed then too:

 https://sourceware.org/ml/gdb-patches/2018-03/msg00204.html

So I wonder whether you already had a patch for that somewhere.

> 
> Also, could you add a test case for this?  I was able to create a simple C++ (not C) program made from an object file built with no debug info:
> 
> int returns_two ()
> {
>   return 2;
> }
> 
> and one built with debug info:
> 
> int returns_two();
> 
> void func()
> {
> }
> 
> int main()
> {
>   func();
>   return 0;
> }
> 
> 
> Putting this breakpoint and running crashes GDB:
> 
> (gdb) b func if (int)returns_two() == 2"
Thanks,
Pedro Alves
  

On 2018-05-29 01:11 PM, Wei-min Pan wrote:
> Since call_function_by_hand_dummy () already calls gdbarch_return_in_first_hidden_param_p() and sets
> hidden_first_param_p accordingly. Instead of passing the deault_return_type and having the target make
> the same call again , I think we should just pass hidden_first_param_p to gdbarch_push_dummy_call()?

I can't really tell, I am a bit confused by gdbarch_return_in_first_hidden_param_p vs
using_struct_return, and the fact that the AArch64 code also checks
language_pass_by_reference on the function's return value type.  You are suggesting
replacing the call to language_pass_by_reference in aarch64_push_dummy_call by
the result of gdbarch_return_in_first_hidden_param_p coming from call_function_by_hand_dummy?
Is it really equivalent?

Simon
  

On 5/29/2018 1:40 PM, Simon Marchi wrote:
> On 2018-05-29 01:11 PM, Wei-min Pan wrote:
>> Since call_function_by_hand_dummy () already calls gdbarch_return_in_first_hidden_param_p() and sets
>> hidden_first_param_p accordingly. Instead of passing the deault_return_type and having the target make
>> the same call again , I think we should just pass hidden_first_param_p to gdbarch_push_dummy_call()?
> I can't really tell, I am a bit confused by gdbarch_return_in_first_hidden_param_p vs
> using_struct_return, and the fact that the AArch64 code also checks
> language_pass_by_reference on the function's return value type.  You are suggesting
> replacing the call to language_pass_by_reference in aarch64_push_dummy_call by
> the result of gdbarch_return_in_first_hidden_param_p coming from call_function_by_hand_dummy?
> Is it really equivalent?
>
> Simon

The traceback below shows that gdbarch_return_in_first_hidden_param_p() 
does
call language_pass_by_reference():


#0  gnuv3_pass_by_reference (type=0xbe0760) at gnu-v3-abi.c:1255
#1  0x000000000052cdc0 in cp_pass_by_reference (type=<optimized out>)
     at cp-abi.c:229
#2  0x00000000005d7428 in language_pass_by_reference (type=<optimized out>)
     at language.c:662
#3  0x00000000005a35c4 in gdbarch_return_in_first_hidden_param_p (
     gdbarch=gdbarch@entry=0xbd6100, type=0xbe0760) at gdbarch.c:2740
#4  0x00000000005bbe7c in call_function_by_hand_dummy (function=0xc319e0,
     default_return_type=default_return_type@entry=0xbe0760, nargs=0,
     args=0xffffffffe118, dummy_dtor=dummy_dtor@entry=0x0,
     dummy_dtor_data=dummy_dtor_data@entry=0x0) at infcall.c:894
  .....

Since I just learned this morning that Alan has been working on this PR, 
maybe it's
best for me  to withdraw the patch that I submitted. Sorry about that.

Weimin