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

  > On 5 Mar 2018, at 16:45, Pedro Alves <palves@redhat.com> wrote:
> 
> On 03/05/2018 03:57 PM, Alan Hayward wrote:
>> 
>> 
>>> On 2 Mar 2018, at 15:18, Joel Brobecker <brobecker@adacore.com> wrote:
>>> 
>>>>>> The cast to (int) is causing this - remove the cast and it finds the type.
>>>>>> I’m assuming that’s causing it to drop the debug info.
>>>>> 
>>>>> It sounds like a bug to me.  If this bug "TYPE_TARGET_TYPE
>>>>> (func_type) becomes NULL caused by cast" is fixed, the GDB segfault
>>>>> will go away accordingly.
>>>>> 
>>>>> To be clear, your patch here is fine to me.  My suggestion is that
>>>>> we'd better dig it deeper.
>>>> 
>>>> Agreed. I’ll raise a new bug, and have a look into it too whilst I’m
>>>> in the area.
>>> 
>>> Having read what Yao said, and looking at the example you gave,
>>> I'm now thinking that one could very well be the cause of the other;
>>> it seems like the cast might indeed be returning a value with
>>> a struct type that's missing the return type. Even a subprogram
>>> which returns nothing has a TYPE_TARGET_TYPE set to a TYPE_CODE_VOID,
>>> right?
>> 
>> Been debugging this a little more (and learnt quite a few things about gdb along the way!)
>> Not sure if this reply is the best place to discuss, or if it should go onto the bug.
>> But….
>> 
>> On x86, for strcmp the type and it's target types, as received in amd64_push_dummy_call are:
>> TYPE_CODE_FUNC -> TYPE_CODE_INT -> 0x0
>> Whereas on aarch64 in aarch64_push_dummy_call we get:
>> TYPE_CODE_FUNC -> 0x0
>> 
>> It turns out this occurs because strcmp has IFUNC vs FUNC differences:
>> 
>> X86:
>> $ readelf -s /lib/x86_64-linux-gnu/libc-2.19.so | grep strcmp
>>  2085: 0000000000087380    60 IFUNC   GLOBAL DEFAULT   12 strcmp@@GLIBC_2.2.5
>> 
>> Aarch64:
>> $ readelf -s /lib/aarch64-linux-gnu/libc-2.23.so | grep strcmp
>>  2043: 0000000000076380   164 FUNC    GLOBAL DEFAULT   12 strcmp@@GLIBC_2.17
>> 
>> 
>> I decided to test this on X86 using a FUNC….
>> 
>> $ readelf -s /lib/x86_64-linux-gnu/libc-2.19.so | grep strlen
>>   769: 0000000000088dd0   436 FUNC    GLOBAL DEFAULT   12 strlen@@GLIBC_2.2.5
>> 
>> Changed my test to do:
>> (gdb) b foo if (int)strlen(name) == 3
>> 
>> ...Now the type received by the target code is TYPE_CODE_FUNC -> 0, the same as aarch64.
>> 
>> However, x86 does not segfault. This is because amd64_push_dummy_call doesn’t bother to
>> check the function parameter.
>> 
>> 
>> Looking into the code, when reading a FUNC, readlf.c : elf_symtab_read() hits:
>> 
>> 		  if (sym->flags & BSF_GNU_INDIRECT_FUNCTION)
>> 		    ms_type = mst_text_gnu_ifunc;
>> 		  else
>> 		    ms_type = mst_text;
>> 
>> Setting the type to mst_text, which eventually causes find_minsym_type_and_address() to
>> set the type to objfile_type (objfile)->nodebug_text_symbol
>> Whereas an IFUNC gets set to objfile_type (objfile)->nodebug_text_gnu_ifunc_symbol.
>> 
>> So, I think probably either:
>> * Everything is correct
>> * mst_text needs handling differently
>> * FUNCs need a new minimal symbol type (mst_text_gnu_func?)
>> (I’m not familiar enough with this part of the code to give a definitive answer).
> 
> Funny enough, I started working on fixing ifunc-related problems early
> last week, but then got sidetracked into other high priority things. I've not
> run into a GDB crash, but just in case, maybe this branch helps:
> 
> https://github.com/palves/gdb/commits/palves/ifunc
> 
> The gnu-ifunc.exp test doesn't pass cleanly on that branch
> because I'm midway through augmenting that testcase to cover
> different scenarios and evaluating whether it's the test that
> needs fixing, or gdb.
> 

Thanks for the branch, gave that it a try, but it has the same error.

Digging a little more into this….

The target type for the *function is set to the type of the function pointer, not the return type.
So, for IFUNC, TYPE_CODE_FUNC target type is TYPE_CODE_INT which gives the type
of the function pointer. For the FUNC, there is no pointer, so the target type of TYPE_CODE_FUNC
is set to null. That makes sense to me, but isn’t immediately obvious.

In call_function_by_hand_dummy() there already exists code to extract the type of the function
return, which is set as target_values_type. Patch below simply passes this through to the
gdbarch_push_dummy_call  method. Updated aarch64 and arm targets to use this.

If this patch is ok for amd64/aarch64/arm, then I will post again with _push_dummy_call()
fixed on every target. (Not done that yet because it’s a lot of tedious copy pasting). Most targets
don’t even look at the return types, so I’m not expecting many issues when I look at them.

Tested on aarch64 only with make check unitest and native_gdbserver.


2018-03-06  Alan Hayward  <alan.hayward@arm.com>

	PR gdb/22736
	* aarch64-tdep.c (aarch64_push_dummy_call): Use passed in return type.
	* amd64-tdep.c (amd64_push_dummy_call): Add new arg.
	* arm-tdep.c (arm_push_dummy_call): Use passed in return type.
	* gdbarch.sh (gdbarch_push_dummy_call): Add return type.
	* gdbarch.h: Regenerate.
	* gdbarch.sh: Likewise.
	* infcall.c (call_function_by_hand_dummy): Pass down return type.
  

Hello Alan,

> The target type for the *function is set to the type of the function
> pointer, not the return type.  So, for IFUNC, TYPE_CODE_FUNC target type
> is TYPE_CODE_INT which gives the type of the function pointer. For the
> FUNC, there is no pointer, so the target type of TYPE_CODE_FUNC is set
> to null. That makes sense to me, but isn’t immediately obvious.

I don't really understand what you mean by that -- maybe it is related
to the '*' before 'function' above? If we're talking about the
target_type for the function, it should be the type of the return value.
At least according to the documentation for that field in gdb_types.h.
But maybe you're talking about something else?

> In call_function_by_hand_dummy() there already exists code to extract
> the type of the function return, which is set as target_values_type.
> Patch below simply passes this through to the gdbarch_push_dummy_call
> method. Updated aarch64 and arm targets to use this.
>
> If this patch is ok for amd64/aarch64/arm, then I will post again with
> _push_dummy_call() fixed on every target. (Not done that yet because
> it’s a lot of tedious copy pasting). Most targets don’t even look at
> the return types, so I’m not expecting many issues when I look at
> them.

Sounds like a good change even if we didn't have this issue, as
it avoids a gdbarch method having to re-compute something that was
already previously computed.

> 2018-03-06  Alan Hayward  <alan.hayward@arm.com>
> 
> 	PR gdb/22736
> 	* aarch64-tdep.c (aarch64_push_dummy_call): Use passed in return type.
> 	* amd64-tdep.c (amd64_push_dummy_call): Add new arg.
> 	* arm-tdep.c (arm_push_dummy_call): Use passed in return type.
> 	* gdbarch.sh (gdbarch_push_dummy_call): Add return type.
> 	* gdbarch.h: Regenerate.
> 	* gdbarch.sh: Likewise.
> 	* infcall.c (call_function_by_hand_dummy): Pass down return type.

Just a minor comment below (besides what you already remarked on about
updating the remaining targets). Please give Pedro some time to comment;
my reviews have not be exactly top notch, lately, and I don't want you
to do useless work because I missed something.

> @@ -1411,20 +1410,11 @@ aarch64_push_dummy_call (struct gdbarch *gdbarch, struct value *function,
>       Rather that change the target interface we call the language code
>       directly ourselves.  */
> 
> -  func_type = check_typedef (value_type (function));
> -
> -  /* Dereference function pointer types.  */
> -  if (TYPE_CODE (func_type) == TYPE_CODE_PTR)
> -    func_type = TYPE_TARGET_TYPE (func_type);
> -
> -  gdb_assert (TYPE_CODE (func_type) == TYPE_CODE_FUNC
> -	      || TYPE_CODE (func_type) == TYPE_CODE_METHOD);
> -
>    /* If language_pass_by_reference () returned true we will have been
>       given an additional initial argument, a hidden pointer to the
>       return slot in memory.  */
> -  return_type = TYPE_TARGET_TYPE (func_type);
> -  lang_struct_return = language_pass_by_reference (return_type);
> +  if (return_type != nullptr)

I think return_type can never be nullptr (from what I understand of
the code in call_function_by_hand_dummy).
  

On 03/09/2018 08:51 AM, Joel Brobecker wrote:

> Just a minor comment below (besides what you already remarked on about
> updating the remaining targets). Please give Pedro some time to comment;
> my reviews have not be exactly top notch, lately, and I don't want you
> to do useless work because I missed something.

Sorry for the constant delays the past couple weeks.  I've been getting
distracted by other things more than usual.  Today I'm trying to finish/post
the ifunc-fixing series I pointed at earlier, and hopefully that will give
me enough background to understand/review this patch (I'm afraid I haven't
really looked at it in any detail).

Thanks,
Pedro Alves
  

Pedro Alves <palves@redhat.com> writes:

> Today I'm trying to finish/post
> the ifunc-fixing series I pointed at earlier, and hopefully that will give
> me enough background to understand/review this patch (I'm afraid I haven't
> really looked at it in any detail).

FWIW, this issue is *not* related to ifunc.  As Alan described in
previous email, ifunc symbol is OK, but normal function symbol's target
type is NULL, because without debug information, GDB doesn't know the
symbol is a function or not.  I thought about it, but I am not confident
that we can set symbol's target type (for example, set it void or int)
in absent of debug information.

Anyway, it is great if you can take a look.
  

> On 9 Mar 2018, at 08:51, Joel Brobecker <brobecker@adacore.com> wrote:

> 

> Hello Alan,

> 

>> The target type for the *function is set to the type of the function

>> pointer, not the return type.  So, for IFUNC, TYPE_CODE_FUNC target type

>> is TYPE_CODE_INT which gives the type of the function pointer. For the

>> FUNC, there is no pointer, so the target type of TYPE_CODE_FUNC is set

>> to null. That makes sense to me, but isn’t immediately obvious.

> 

> I don't really understand what you mean by that -- maybe it is related

> to the '*' before 'function' above? If we're talking about the

> target_type for the function, it should be the type of the return value.

> At least according to the documentation for that field in gdb_types.h.

> But maybe you're talking about something else?


Agreed that the documentation says “type” should contain the type of the
function return.

I’m not quite sure on the terminology here, but, what the gdb code is picking
up and placing into type is the type of the function entry in the elf file.
The code in find_minsym_type_and_address() ensures that a FUNC will always
have the type nodebug_text_gnu_ifunc_symbol and an IFUNC will always have
the type nodebug_text_symbol.  This is completely unrelated to the return type
of the function, which is only resolved much later in call_function_by_hand_dummy().

In an ideal world, maybe the common code needs a bit of a rewrite:
The function structure needs to contain both the type of the elf entry and the
type of the return value. But, the existing value* and function* structures don’t
really support adding extra fields. I wasn’t really comfortable enough will this part
of the code to meddle with that.

Happy with the other comments too. Thanks!


> On 9 Mar 2018, at 16:04, Pedro Alves <palves@redhat.com> wrote:

> 

> Sorry for the constant delays the past couple weeks.  I've been getting

> distracted by other things more than usual.  Today I'm trying to finish/post

> the ifunc-fixing series I pointed at earlier, and hopefully that will give

> me enough background to understand/review this patch (I'm afraid I haven't

> really looked at it in any detail).

> 


Ok, happy to wait until you have some time. I’m looking at pr/22943 which also
might help with my understanding of this one a little more.

> 

> On 9 Mar 2018, at 16:44, Yao Qi <qiyaoltc@gmail.com> wrote:

> 

> FWIW, this issue is *not* related to ifunc.  As Alan described in

> previous email, ifunc symbol is OK, but normal function symbol's target

> type is NULL, because without debug information, GDB doesn't know the

> symbol is a function or not.  I thought about it, but I am not confident

> that we can set symbol's target type (for example, set it void or int)

> in absent of debug information.

> 


I’d just add that even with IFUNC the type will always be set to a pointer to an
Int, regardless of the return type of the function.




Alan.
  

On 03/09/2018 07:11 PM, Alan Hayward wrote:

>> On 9 Mar 2018, at 16:44, Yao Qi <qiyaoltc@gmail.com> wrote:
>>
>> FWIW, this issue is *not* related to ifunc.  As Alan described in
>> previous email, ifunc symbol is OK, but normal function symbol's target
>> type is NULL, because without debug information, GDB doesn't know the
>> symbol is a function or not.  I thought about it, but I am not confident
>> that we can set symbol's target type (for example, set it void or int)
>> in absent of debug information.
>>

And how are we calling a function if we don't know its return type?
GDB won't let you, unless you add the cast.  And then the question is,
why are we losing the cast-to type then?  _That_ should the be return
type used, it sounds to me (but I still haven't reviewed in detail :-/ ).

> 
> I’d just add that even with IFUNC the type will always be set to a pointer to an
> Int, regardless of the return type of the function.

Actually, I've changed that today in my series.  /me almost done...

Thanks,
Pedro Alves