From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id A507A3858C36 for ; Thu, 27 Jul 2023 09:09:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A507A3858C36 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1690448993; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=PQInpFqCSppljHcH8+KfGDRVy+92vluBeyhTV/mM4IU=; b=Kh8PKoQa5yx1OSTVr/3UIAMFbkPFtS0wei2hZC6CLG5hgIg2+Gq0y5DOyZ/Cih23Ns+8rR gtSvCwJcUU3hOYadzDRdzvwHvtsxUIoMIWZtSOsk0KtNiacojlsh93sCrZDwzSuRjEAPT3 Xkfg0KDa4ZQwlrpQZRWkPtTqgTv3OQE= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-426-1P8ADgMxOgulVTCQTnt5Kg-1; Thu, 27 Jul 2023 05:09:52 -0400 X-MC-Unique: 1P8ADgMxOgulVTCQTnt5Kg-1 Received: by mail-wr1-f69.google.com with SMTP id ffacd0b85a97d-316f5d82bf4so441888f8f.3 for ; Thu, 27 Jul 2023 02:09:51 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690448991; x=1691053791; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=PQInpFqCSppljHcH8+KfGDRVy+92vluBeyhTV/mM4IU=; b=BYh4Y6QH780+qoS6tU/9VYcWwaerGSUNyWrLA+fY8lQhjUpRG/j3QdDtTMoaVJvCEv /fTiy2lES47GoNtf7FYpVRswYnAZg5/GbnrXg/qq0ZbkKxXL8AdPeMZnN/nh4oOSRJG9 VtjIJy0A/c2Hse3+VGFA4xe2uDLP8yWpf5g81hLPgEJhCQaf0nthz++LsG74OTtrPKjZ ML2jfG64vWyk7m6UzNJY8fWexpGaBHtQVW/ektFv0J6pEEeR03DKo3eujbGbMgkYyG6f nn7oDmYKcvQ/Vsgv3GqephFyzgfi5Yno4/QW8is5D/ZVNxzJnaEiXbCZSL3BSGCMvmRV DcWA== X-Gm-Message-State: ABy/qLbKJkxLeDTpdPtR0m68UwAQHSq6mfWvg/cuymsWxTJMjAMTid3O r5G3svgmvWbyJWsOzNUhYx0U6i6fUSRJ/S/RzOelJq65dpWcdeFKh+UwQ7sNcqOLYA3kcQs0ZjJ OafKHq86bL1sn26DtbSZATQ== X-Received: by 2002:a5d:658b:0:b0:313:f5e9:13ec with SMTP id q11-20020a5d658b000000b00313f5e913ecmr1170815wru.68.1690448990810; Thu, 27 Jul 2023 02:09:50 -0700 (PDT) X-Google-Smtp-Source: APBJJlE2N5T+1/faOMFtET+X4oPcnt4e/5MWy8lEzIrK4HBZQo+LJIuTHqwA9fjcfLrWUNJBgTIFtA== X-Received: by 2002:a5d:658b:0:b0:313:f5e9:13ec with SMTP id q11-20020a5d658b000000b00313f5e913ecmr1170799wru.68.1690448990372; Thu, 27 Jul 2023 02:09:50 -0700 (PDT) Received: from [192.168.0.129] (ip-94-112-225-44.bb.vodafone.cz. [94.112.225.44]) by smtp.gmail.com with ESMTPSA id r5-20020a056000014500b00314367cf43asm1378128wrx.106.2023.07.27.02.09.49 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 27 Jul 2023 02:09:49 -0700 (PDT) Message-ID: <27931c60-cdee-e430-13bf-37cbbcc926aa@redhat.com> Date: Thu, 27 Jul 2023 11:09:48 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH v3 1/4] gdb, dwarf: add support for DW_AT_trampoline in DWARF reader To: Abdul Basit Ijaz , gdb-patches@sourceware.org Cc: JiniSusan.George@amd.com, tom@tromey.com, eliz@gnu.org, Nils-Christian Kempke References: <20230710225643.32280-1-abdul.b.ijaz@intel.com> <20230710225643.32280-2-abdul.b.ijaz@intel.com> From: Bruno Larsen In-Reply-To: <20230710225643.32280-2-abdul.b.ijaz@intel.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-9.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 11/07/2023 00:56, Abdul Basit Ijaz via Gdb-patches wrote: > From: Nils-Christian Kempke > > 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 > --- > 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