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

["Ijaz, Abdul B" <abdul.b.ijaz@intel.com>]

Hi Tom,

Thanks for the feedback. Added replies below.

Abdul> +      if (in_trampoline)

Tom > 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.
Will fix in V5.

Abdul> +	  for (int i = 0; i < MAX_TRAMPOLINE_CHAIN_SIZE
Abdul> +			  && in_trampoline_function (real_stop_pc); ++i)
Tom > Normally we'd split at the ';':
Will fix in V5.

Abdul> +		if (real_stop_pc == 0x0)
Tom>Just '0', not '0x0'.
Will fix both instances in V5.

Tom> I guess the language stuff is there to work around a GCC deficiency, in that it does not emit DW_AT_trampoline.
Not sure about deficiency but I think there are Signal trampolines which need to be handled separately for every arch while for thunks as far as I understand for its handling there could be  language stuff which might be needed for different languages.

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

All these trampoline cases are really different way of detecting and the processing them and I am not sure compiler decision about DW_AT_trampoline. So far able to see them only for Intel Fortran compilers for all the function calls.  For unifying need better understanding of existing trampolines and I think I am lacking the understanding of the implementation of other trampolines to handle thunks or signal trampolines. Now regarding erroneous stop if I understand rightly in case of some targets where compiler emit thunks for trampoline calls and also it emit trampoline dwarf for same targets then I think the trampoline detected using thunks handling  should be done first or which ever I processed first should process the trampolines. In case of DW_AT_trampoline it is set for the respective frame and if it is not detected in case those frames were processed already then nothing should be affected, so should not be an issue. But if we want to make sure then have to try on some target not sure how so that I may verify to confirm if  it will work fine in such case.

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

So skip_trampoline_functions is so far only handling the skipping case for trampolines indicated by DW_AT_trampoline and not for any other case of these thunks emitted by compiler or by signal trampolines. Since this case is pretty much defined case for subroutines calling the target subroutine but not sure about others but may be these solib calls filtering may also be handled with the same. Please let me know if we want to filter other trampolines cases using the same filter. Mostly it should be "on" case but DW_AT_trampoline  could be interesting for user if compiler emit it for virtual functions and user want to see all the trampoline calls so here there could be some cases for end user to see those trampoline calls but for thunks or rest I don’t know so let me know then I can asses of using the filter option for other trampolines.

Thanks  & Best Regards,
Abdul Basit

-----Original Message-----
From: Tom Tromey <tom@tromey.com> 
Sent: Wednesday, August 2, 2023 10:34 PM
To: Abdul Basit Ijaz via Gdb-patches <gdb-patches@sourceware.org>
Cc: Ijaz, Abdul B <abdul.b.ijaz@intel.com>; JiniSusan.George@amd.com; tom@tromey.com; eliz@gnu.org; blarsen@redhat.com; Nils-Christian Kempke <nils-christian.kempke@intel.com>
Subject: Re: [PATCH v4 3/4] gdb/infrun: handle stepping through functions with DW_AT_trampoline

>>>>> "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 
Abdul> trampoline functions marked via the DW_AT_trampoline in DWARF.  
Abdul> The attribute can be emitted by the compiler for certain 
Abdul> subroutines/inlined subroutines 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, 
Abdul> 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
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928