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

Joel Brobecker
2018-03-09T08:51:28Z

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

--
Joel