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

[Bruno Larsen <blarsen@redhat.com>]

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_target
>   #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