RE: [PATCH v3 1/4] gdb, dwarf: add support for DW_AT_trampoline in DWARF reader

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

Thanks a lot Bruno for your feedback.

Can you please have a look on the replies if its fine with you otherwise will I update accordingly in V4 patch series.

1)
> As was mentioned in v1 of this patch, this bit gets the "regular" name of the function ...
Yes, this should have been fixed. Somehow missed it, will first try to read linkage name and they regular. Modified locally so should be fixed in next series.

2)
> Why not just have

>    TYPE_FUNC_FLAGS (new_type) = TYPE_FUNC_FLAGS (type);
Yes this make sense, changed locally to replace 2 lines for return and trampoline type with this line and now only need to test so hopefully should be updated in v4 series.

3) 
>> +    struct trampoline_target *self_trampoline_target;

>This member should have a comment describing it.

>Also, why did you call it self_trampoline_target instead of trampoline_target? The only other member prefixed with self_ in this struct is self_type, and that has it because when a method wants to know the type pointed at by the "self" or "this" variable, it will check that pointer, not due to self referencing stuff, if I understood it correctly.

Intention behind self_ is only that the trampoline calls itself are just for calling to the target function. So each trampoline entry is just pointing to the target call. Not sure if self_* usage is restricted for this where source is just referencing to the target. So please let me know if adding comment with this information will be sufficient otherwise I will remove the self_  suffix from the variable name.

4)
> After some time looking into how GDB calculates names and what not, I don't think calling this "physname" is a good idea, I think it would be better to call it TRAMPOLINE_TARGET_MANGLED_NAME.

> My reasoning for that is that physname is not some well defined thing, it is a name that a GDB developer came up with to call what happens when GDB needs to calculate mangled names ourselves because the compiler didn't add a linkage name, as far as I understood it. Since the struct trampoline_target doesn't care about how a mangled name was calculated, and mangled name is a well defined thing while physname is not, I think its better to use the latter.

Since as per (1) comment above we are first going to read mangled and if not then demangled name. So I would then just prefer to call it TRAMPOLINE_TARGET_NAME, hopefully it would be fine with you.

5)
> s/An/A
>> +    TRAMPOLINE_TARGET_FLAG,
Will be fixed in V4 series.

6) 
> The struct and all methods in it should also have comments.
>> +struct trampoline_target
Will add in V4 series.

7) 
> Minor nit, but I'd move this up above type_tail_call_list, just to keep all func_flag stuff together.
Will be updated accordingly in V4 series.

Thanks & Best Regards,
Abdul Basit

-----Original Message-----
From: Bruno Larsen <blarsen@redhat.com> 
Sent: Thursday, July 27, 2023 11:10 AM
To: Ijaz, Abdul B <abdul.b.ijaz@intel.com>; gdb-patches@sourceware.org
Cc: JiniSusan.George@amd.com; tom@tromey.com; eliz@gnu.org; Nils-Christian Kempke <nils-christian.kempke@intel.com>
Subject: Re: [PATCH v3 1/4] gdb, dwarf: add support for DW_AT_trampoline in DWARF reader

On 11/07/2023 00:56, Abdul Basit Ijaz via Gdb-patches wrote:
> From: Nils-Christian Kempke <nils-christian.kempke@intel.com>
>
> DW_AT_trampoline can be used to describe compiler generated functions 
> that serve some intermediary purpose on making a call to another 
> function.  A compiler can emit this tag in order to help a debugger 
> hide the trampolines from a user.
>
> The attribute is only applicable to DW_TAG_subroutine and 
> DW_TAG_inlined_subroutine tags.  It contains information about the 
> trampoline target either as a reference to its DIE, as its address or 
> its name.  DW_AT_trampoline can also be a flag indicating that the DIE 
> is a trampoline or not without specifying the target (e.g. if it is 
> unknown).
>
> This patch adds support to GDB for reading the DW_AT_trampoline 
> attribute.  It stores the attribute and its value in the type_specific 
> part of a GDB type.  This patch is implemented in preparation of the 
> following patches, which will add a mechanism to hide DW_AT_trampoline 
> subroutines from the user.

Sorry for jumping in late for the review. If I say something that's been previously agreed upon, feel free to ignore me :)

Thanks for working on this! Some comments and nits inlined.

> 2023-07-10 Nils-Christian Kempke <nils-christian.kempke@intel.com>
> ---
>   gdb/dwarf2/read.c |  45 +++++++++++++++++++-
>   gdb/gdbtypes.c    |  35 +++++++++++++++-
>   gdb/gdbtypes.h    | 103 +++++++++++++++++++++++++++++++++++++++++++---
>   3 files changed, 176 insertions(+), 7 deletions(-)
>
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 
> 3508f2c29ee..1cfa8178d94 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -14657,6 +14657,49 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
>     if (prototyped_function_p (die, cu))
>       ftype->set_is_prototyped (true);
>   
> +  /* If this is a trampoline function store it and its target here.  
> +*/
> +  attr = dwarf2_attr (die, DW_AT_trampoline, cu);
> +  if (attr != nullptr)
> +    {
> +      TYPE_FUNC_FLAGS (ftype) |= FUNC_TYPE_TRAMPOLINE;
> +      TYPE_TRAMPOLINE_TARGET (ftype)
> +	= (trampoline_target *) TYPE_ZALLOC (ftype,
> +					     sizeof (trampoline_target));
> +
> +      /* A DW_AT_trampoline can be either an address, a flag, a reference or a
> +	 string.  */
> +      if (attr->form_is_string ())
> +	TYPE_TRAMPOLINE_TARGET (ftype)->set_target_physname
> +	  (attr->as_string ());
> +      else if (attr->form_is_ref ())
> +	{
> +	  die_info *target_die;
> +	  dwarf2_cu *target_cu = cu;
> +
> +	  target_die = follow_die_ref (die, attr, &target_cu);
> +	  const char *target_name = dwarf2_name (target_die, target_cu);
As was mentioned in v1 of this patch, this bit gets the "regular" name of the function. This could be a problem if there are 2 functions with the same in different contexts (say in and out of a namespace).  I would suggest that you first attempt to take the linkage_name and only if that doesn't exist do you take the name of the function. This way, if GDB misidentifies a function it would probably be a compiler error, not adding the linkage name.
> +	  if (target_name == nullptr)
> +	    {
> +	      complaint (_("DW_AT_trampoline target DIE has no name for"
> +			 "referencing DIE %s [in module %s]"),
> +			 sect_offset_str (die->sect_off),
> +			 objfile_name (objfile));
> +	    }
> +	  TYPE_TRAMPOLINE_TARGET (ftype)->set_target_physname (target_name);
> +	}
> +      else if (attr->form_is_unsigned ())
> +	TYPE_TRAMPOLINE_TARGET (ftype)->set_target_flag (attr->as_boolean ());
> +      else
> +	{
> +	  unrelocated_addr target_addr_reloc = attr->as_address ();
> +	  CORE_ADDR target_addr
> +	    = cu->per_objfile->relocate (target_addr_reloc);
> +	  target_addr = gdbarch_adjust_dwarf2_addr (objfile->arch (),
> +						    target_addr);
> +	  TYPE_TRAMPOLINE_TARGET (ftype)->set_target_physaddr (target_addr);
> +	}
> +    }
> +
>     /* Store the calling convention in the type if it's available in
>        the subroutine die.  Otherwise set the calling convention to
>        the default value DW_CC_normal.  */ @@ -14674,7 +14717,7 @@ 
> read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
>        if the DWARF producer set that information.  */
>     attr = dwarf2_attr (die, DW_AT_noreturn, cu);
>     if (attr && attr->as_boolean ())
> -    TYPE_NO_RETURN (ftype) = 1;
> +    TYPE_FUNC_FLAGS (ftype) |= FUNC_TYPE_NO_RETURN;
>   
>     /* We need to add the subroutine type to the die immediately so
>        we don't infinitely recurse when dealing with parameters diff 
> --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c index c5272979cb9..7355317f7db 
> 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -5091,6 +5091,33 @@ print_fixed_point_type_info (struct type *type, int spaces)
>   	      type->fixed_point_scaling_factor ().str ().c_str ());
>   }
>   
> +/* Print the contents of the TYPE's self_trampoline_target, assuming that its
> +   type-specific kind is TYPE_SPECIFIC_FUNC and is_trampoline is not 
> +0.  */ static void print_trampoline_target_info (struct type *type, 
> +int spaces) {
> +  switch (TYPE_TRAMPOLINE_TARGET (type)->target_kind ())
> +    {
> +    case TRAMPOLINE_TARGET_PHYSADDR:
> +      gdb_printf ("%*starget physaddr: 0x%s\n", spaces + 2, "",
> +		  print_core_address (type->arch_owner (),
> +				      TYPE_TRAMPOLINE_TARGET (type)
> +				      ->target_physaddr ()));
> +      break;
> +    case TRAMPOLINE_TARGET_PHYSNAME:
> +      gdb_printf ("%*starget physname: %s\n", spaces + 2, "",
> +		  TYPE_TRAMPOLINE_TARGET (type)->target_physname ());
> +      break;
> +    case TRAMPOLINE_TARGET_FLAG:
> +      gdb_printf ("%*starget flag: %d\n", spaces + 2, "",
> +		  TYPE_TRAMPOLINE_TARGET (type)->target_flag ());
> +      break;
> +    default:
> +      gdb_assert_not_reached ("unhandled trampoline target kind");
> +      break;
> +    }
> +}
> +
>   static struct obstack dont_print_type_obstack;
>   
>   /* Print the dynamic_prop PROP.  */
> @@ -5417,6 +5444,10 @@ recursive_dump_type (struct type *type, int spaces)
>   	gdb_printf ("%*scalling_convention %d\n", spaces, "",
>   		    TYPE_CALLING_CONVENTION (type));
>   	/* tail_call_list is not printed.  */
> +	gdb_printf ("%*sfunc_type_flags %u\n", spaces, "",
> +		    (unsigned int) TYPE_FUNC_FLAGS (type));
> +	if (TYPE_IS_TRAMPOLINE (type))
> +	  print_trampoline_target_info (type, spaces);
>   	break;
>   
>         case TYPE_SPECIFIC_SELF_TYPE:
> @@ -5633,8 +5664,10 @@ copy_type_recursive (struct type *type, htab_t copied_types)
>       case TYPE_SPECIFIC_FUNC:
>         INIT_FUNC_SPECIFIC (new_type);
>         TYPE_CALLING_CONVENTION (new_type) = TYPE_CALLING_CONVENTION (type);
> -      TYPE_NO_RETURN (new_type) = TYPE_NO_RETURN (type);
> +      TYPE_FUNC_FLAGS (new_type) |= TYPE_NO_RETURN (type);
>         TYPE_TAIL_CALL_LIST (new_type) = NULL;
> +      TYPE_FUNC_FLAGS (new_type) |= TYPE_IS_TRAMPOLINE (type);

Why not just have

    TYPE_FUNC_FLAGS (new_type) = TYPE_FUNC_FLAGS (type);

?

> +      TYPE_TRAMPOLINE_TARGET (new_type) = TYPE_TRAMPOLINE_TARGET 
> + (type);
>         break;
>       case TYPE_SPECIFIC_FLOATFORMAT:
>         TYPE_FLOATFORMAT (new_type) = TYPE_FLOATFORMAT (type); diff 
> --git a/gdb/gdbtypes.h b/gdb/gdbtypes.h index aedaf53cd5d..a1789223cd5 
> 100644
> --- a/gdb/gdbtypes.h
> +++ b/gdb/gdbtypes.h
> @@ -106,6 +106,21 @@ enum type_instance_flag_value : unsigned
>   
>   DEF_ENUM_FLAGS_TYPE (enum type_instance_flag_value, 
> type_instance_flags);
>   
> +/* * Define flags for function types.  */ enum func_type_flag_value : 
> +unsigned {
> +  /* * Flag indicates, whether this function normally returns to its
> +     caller.  It is set from the DW_AT_noreturn attribute if set on
> +     the DW_TAG_subprogram.  */
> +  FUNC_TYPE_NO_RETURN = (1 << 0),
> +
> +  /* * Flag is used for functions marked with DW_AT_trampoline.  These
> +     are compiler generated wrappers that should be hidden from the 
> +user.  */
> +  FUNC_TYPE_TRAMPOLINE = (1 << 1)
> +};
> +
> +DEF_ENUM_FLAGS_TYPE (enum func_type_flag_value, func_type_flags);
> +
>   /* * Not textual.  By default, GDB treats all single byte integers as
>      characters (or elements of strings) unless this flag is set.  */
>   
> @@ -1730,11 +1745,9 @@ struct func_type
>   
>       ENUM_BITFIELD (dwarf_calling_convention) calling_convention : 8;
>   
> -    /* * Whether this function normally returns to its caller.  It is
> -       set from the DW_AT_noreturn attribute if set on the
> -       DW_TAG_subprogram.  */
> +    /* * For storing function types defined in eunm 
> + func_type_flag_value.  */
>   
> -    unsigned int is_noreturn : 1;
> +    func_type_flags flags;
>   
>       /* * Only those DW_TAG_call_site's in this function that have
>          DW_AT_call_tail_call set are linked in this list.  Function 
> @@ -1749,6 +1762,78 @@ struct func_type
>          contains the method.  */
>   
>       struct type *self_type;
> +
> +    struct trampoline_target *self_trampoline_target;

This member should have a comment describing it.

Also, why did you call it self_trampoline_target instead of trampoline_target? The only other member prefixed with self_ in this struct is self_type, and that has it because when a method wants to know the type pointed at by the "self" or "this" variable, it will check that pointer, not due to self referencing stuff, if I understood it correctly.

> +  };
> +
> +/* The kind of location held by this call site target.  */ enum 
> +trampoline_target_kind
> +  {
> +    /* An address.  */
> +    TRAMPOLINE_TARGET_PHYSADDR,
> +    /* A (mangled) name.  */
> +    TRAMPOLINE_TARGET_PHYSNAME,

After some time looking into how GDB calculates names and what not, I don't think calling this "physname" is a good idea, I think it would be better to call it TRAMPOLINE_TARGET_MANGLED_NAME.

My reasoning for that is that physname is not some well defined thing, it is a name that a GDB developer came up with to call what happens when GDB needs to calculate mangled names ourselves because the compiler didn't add a linkage name, as far as I understood it. Since the struct trampoline_target doesn't care about how a mangled name was calculated, and mangled name is a well defined thing while physname is not, I think its better to use the latter.

> +    /* An flag (target is unknown).  */
s/An/A
> +    TRAMPOLINE_TARGET_FLAG,
> +  };
> +
> +struct trampoline_target
The struct and all methods in it should also have comments.
> +  {
> +    trampoline_target_kind target_kind () const
> +    {
> +      return m_target_kind;
> +    }
> +
> +    void set_target_physaddr (CORE_ADDR physaddr)
> +    {
> +      m_target_kind = TRAMPOLINE_TARGET_PHYSADDR;
> +      m_trampoline_target.physaddr = physaddr;
> +    }
> +
> +    CORE_ADDR target_physaddr () const
> +    {
> +      gdb_assert (m_target_kind == TRAMPOLINE_TARGET_PHYSADDR);
> +      return m_trampoline_target.physaddr;
> +    }
> +
> +    void set_target_physname (const char *physname)
> +    {
> +      m_target_kind = TRAMPOLINE_TARGET_PHYSNAME;
> +      m_trampoline_target.physname = physname;
> +    }
> +
> +    const char *target_physname () const
> +    {
> +      gdb_assert (m_target_kind == TRAMPOLINE_TARGET_PHYSNAME);
> +      return m_trampoline_target.physname;
> +    }
> +
> +    void set_target_flag (bool flag)
> +    {
> +      m_target_kind = TRAMPOLINE_TARGET_FLAG;
> +      m_trampoline_target.flag = flag;
> +    }
> +
> +    bool target_flag () const
> +    {
> +      gdb_assert (m_target_kind == TRAMPOLINE_TARGET_FLAG);
> +      return m_trampoline_target.flag;
> +    }
> +
> +  private:
> +
> +    union
> +    {
> +      /* Address.  */
> +      CORE_ADDR physaddr;
> +      /* Mangled name.  */
> +      const char *physname;
> +      /* Flag.  */
> +      bool flag;
> +    } m_trampoline_target;
> +
> +    /* * Discriminant for union m_trampoline_target.  */
> +    ENUM_BITFIELD (trampoline_target_kind) m_target_kind : 2;
>     };
>   
>   /* The type-specific info for TYPE_CODE_FIXED_POINT types.  */ @@ 
> -1891,8 +1976,16 @@ extern void set_type_vptr_basetype (struct type *, struct type *);
>   #define TYPE_GNAT_SPECIFIC(thistype) TYPE_MAIN_TYPE(thistype)->type_specific.gnat_stuff
>   #define TYPE_DESCRIPTIVE_TYPE(thistype) TYPE_GNAT_SPECIFIC(thistype)->descriptive_type
>   #define TYPE_CALLING_CONVENTION(thistype) 
> TYPE_MAIN_TYPE(thistype)->type_specific.func_stuff->calling_convention
> -#define TYPE_NO_RETURN(thistype) 
> TYPE_MAIN_TYPE(thistype)->type_specific.func_stuff->is_noreturn
> +#define TYPE_FUNC_FLAGS(thistype) \
> +  TYPE_MAIN_TYPE(thistype)->type_specific.func_stuff->flags
> +#define TYPE_NO_RETURN(thistype) \
> +  (TYPE_MAIN_TYPE(thistype)->type_specific.func_stuff->flags \
> +   & FUNC_TYPE_NO_RETURN)
>   #define TYPE_TAIL_CALL_LIST(thistype) 
> TYPE_MAIN_TYPE(thistype)->type_specific.func_stuff->tail_call_list
> +#define TYPE_IS_TRAMPOLINE(thistype) \
> +  (TYPE_MAIN_TYPE(thistype)->type_specific.func_stuff->flags \
> +   & FUNC_TYPE_TRAMPOLINE)
Minor nit, but I'd move this up above type_tail_call_list, just to keep all func_flag stuff together.
> +#define TYPE_TRAMPOLINE_TARGET(thistype) 
> +TYPE_MAIN_TYPE(thistype)->type_specific.func_stuff->self_trampoline_t
> +arget
>   #define TYPE_BASECLASS(thistype,index) ((thistype)->field (index).type ())
>   #define TYPE_N_BASECLASSES(thistype) TYPE_CPLUS_SPECIFIC(thistype)->n_baseclasses
>   #define TYPE_BASECLASS_NAME(thistype,index) (thistype->field 
> (index).name ())


--
Cheers,
Bruno

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