Re: [PATCH v4 3/4] gdb/infrun: handle stepping through functions with DW_AT_trampoline

[Tom Tromey <tom@tromey.com>]

>>>>> "Abdul" == Abdul Basit Ijaz via Gdb-patches <gdb-patches@sourceware.org> writes:

Abdul> From: Nils-Christian Kempke <nils-christian.kempke@intel.com>
Abdul> This patch makes infrun continue stepping into and through trampoline
Abdul> functions marked via the DW_AT_trampoline in DWARF.  The attribute can
Abdul> be emitted by the compiler for certain subroutines/inlined subroutines
Abdul> that are compiler generated and should be hidden from a user.

Thanks for the patch.

infrun isn't really my area but I do have some nits.

Abdul> +      bool in_trampoline = skip_trampoline_functions
Abdul> +			   && in_trampoline_function (stop_pc);
Abdul> +
Abdul> +      if (in_trampoline)

The assignment is not formatted correctly, but it seems to me that
there's no need for this variable at all, and the expression can be
inlined into the 'if' -- which will also fix the formatting.

Abdul> +	  real_stop_pc = find_function_trampoline_target (stop_pc);
Abdul> +
Abdul> +	  for (int i = 0; i < MAX_TRAMPOLINE_CHAIN_SIZE
Abdul> +			  && in_trampoline_function (real_stop_pc); ++i)

Normally we'd split at the ';':

    for (int i = 0;
         i < MAX_TRAMPOLINE_CHAIN_SIZE
         && in_trampoline_function (real_stop_pc);
         ++i)

... with the middle expression being all on one line if it fits.

Abdul> +	    {
Abdul> +		real_stop_pc = find_function_trampoline_target (real_stop_pc);
Abdul> +		/* Exit if find_function_trampoline_target failed to find the
Abdul> +		   trampoline target.  Do not try to resolve the trampolines
Abdul> +		   in this case.  */
Abdul> +		if (real_stop_pc == 0x0)

Just '0', not '0x0'.

Abdul> +	  /* If we failed to find a target we will just single step in the
Abdul> +	     hope of leaving the trampoline again soon.  */
Abdul> +	  if (real_stop_pc == 0x0)

Ditto.

Abdul> +      if (real_stop_pc == 0)
Abdul> +	real_stop_pc = skip_language_trampoline (frame, stop_pc);
Abdul>        if (real_stop_pc == 0)
Abdul>  	real_stop_pc = gdbarch_skip_trampoline_code (gdbarch, frame, stop_pc);

This is pre-existing code, but I wonder how all these different pieces
interact, or are supposed to interact.

I guess the language stuff is there to work around a GCC deficiency, in
that it does not emit DW_AT_trampoline.

The gdbarch stuff... I don't know but I see a lot of arches implement
this (though mostly deferring to the solib code).

Anyway, is it possible to have a gdbarch or c++ trampoline then call a
DW_AT_trampoline function?  If so then wouldn't we end up with an
erroneous stop?

I'm just wondering if these ought to be unified in some way, like say
look for any sort of trampoline target in the loop.

Also LOL at the gnu-v3-abi.c code:

  if (thunk_name == NULL || strstr (thunk_name, " thunk to ") == NULL)
    return 0;

It also seems a bit weird that skip_trampoline_functions only affects
this one subset of trampoline functions.

Tom