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

Alan Hayward
2018-10-09T17:50:48Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3201 bytes --]

> 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.

\x16º&Öéj×!zÊÞ¶êç×yÓÙb²Ö«r\x18\x1dn–­r\x17¬