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

[Joel Brobecker <brobecker@adacore.com>]

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