Re: [PATCH v2 0/2] Aarch64: Fix segfault when casting dummy calls

[Alan Hayward <Alan.Hayward@arm.com>]

> On 9 Oct 2018, at 17:10, Pedro Alves <palves@redhat.com> wrote:
> 
> On 10/01/2018 04:52 PM, Alan Hayward wrote:
>> This is a reworking of a patch I posted in March.
>> V1 had a long discussion which was then paused to wait for
>> Pedro's IFUNC rewrite.
>> 
>> 
>> Prevent the int cast in the following causing a segfault on aarch64:
>> (gdb) b foo if (int)strcmp(name,"abc") == 0
>> (gdb) run
>> 
>> 
>> This is because to aarch64_push_dummy_call determines the return type
>> of the function and then does not check for null pointer.
>> 
>> A null pointer for the return type means either 1) the call has a
>> cast or 2) an error has occured.
> 
> I'd think that "1) the call has a cast" is not accurate.
> If the called function has debug info, then GDB will know
> it's return type.  The issue is that the called function may
> not have debug information, and then GDB does not know
> its return type (so its NULL), and then the only way to
> call the function is to add the cast.  Right?
> 

That makes sense. I’d add that in the above example I’m able to do the
break without a cast and gdb does not segfault - the return type
of the function comes back as an int. 
 
> It kind of sounds like IFUNCs were a red herring then.  :-/

Yeah.

> 
>> You can see this in infcall.c:call_function_by_hand_dummy():
>> 
>>  CORE_ADDR funaddr = find_function_addr (function, &values_type, &ftype);
>> 
>>  if (values_type == NULL)
>>    values_type = default_return_type;
>>  if (values_type == NULL)
>>    {
>>      const char *name = get_function_name (funaddr,
>> 					    name_buf, sizeof (name_buf));
>>      error (_("'%s' has unknown return type; "
>> 	       "cast the call to its declared return type"),
>> 	     name);
>>    }
>> 
>> In aarch64_push_dummy_call we do not have default_return_type, so cannot
>> determine between the two cases.
>> 
>> (In addition, aarch64_push_dummy_call incorrectly resolves the return
>> type for IFUNC).
> 
> Can you expand a bit on this IFUNC remark?

Have a look at find_function_addr in infcall.c there is a section of code
beginning "if (TYPE_GNU_IFUNC (ftype))”. That is missing from the aarch64
code.

> 
> 
>> However, aarch64_push_dummy_call only requires the return value in order
>> to calculate lang_struct_return ... which has previously been calculated
>> in the caller:
>> 
>>     This is slightly awkward, ideally the flag "lang_struct_return"
>>     would be passed to the targets implementation of push_dummy_call.
>>     Rather that change the target interface we call the language code
>>     directly ourselves.
>> 
> 
> Ah, nice, the solution was right there.  :-)
> 
>> The fix is simple:
>> Patch 1: Update gdbarch interface to pass lang_struct_return.
>> Patch 2: Remove incorrect code and use the passed in lang_struct_return.
>> 
> 
> Since cover letters don't end up in git, this info should be
> somehow migrated into the commit logs of the two patches.

Ok, I’ll merge it into the logs in the next version.

I’ll take a look at your other comments tomorrow.

Thanks for the review,
Alan.