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.129.124]) by sourceware.org (Postfix) with ESMTPS id 0B91F3858C50 for ; Tue, 29 Mar 2022 14:02:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0B91F3858C50 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-505-gB2zoWwLNJeVZJK4LjMneQ-1; Tue, 29 Mar 2022 10:02:21 -0400 X-MC-Unique: gB2zoWwLNJeVZJK4LjMneQ-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id F1781803D65; Tue, 29 Mar 2022 14:02:20 +0000 (UTC) Received: from [10.97.116.56] (ovpn-116-56.gru2.redhat.com [10.97.116.56]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4DEBA40D1B9B; Tue, 29 Mar 2022 14:02:17 +0000 (UTC) Message-ID: <66ca57f9-c3b8-4aeb-f07c-c3c254dfdea6@redhat.com> Date: Tue, 29 Mar 2022 11:02:13 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH] Debug support for global alias variable To: "Natarajan, Kavitha" , "Natarajan, Kavitha via Gdb-patches" Cc: "George, Jini Susan" , "Parasuraman, Hariharan" References: From: Bruno Larsen In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.11.54.2 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: 7bit X-Spam-Status: No, score=-12.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, 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.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Mar 2022 14:02:42 -0000 Hello Kavitha, Thanks for the quick reply On 3/28/22 12:14, Natarajan, Kavitha wrote: > [AMD Official Use Only] > > Hello Bruno, > > Thanks for reviewing the patch. It helps a lot. Along with my responses to your questions, > I have also attached the new patch with the changes. Please review. >> why is this function returning 1 or 0 as boolean, instead of the >> function returning bool? > > The function shall return bool instead of int. Fixed in the new patch. > >> Is this "else" path being exercised anywhere? For this simple code: >> >> namespace n{ >> int i; >> int j; >> void f(){ } >> } >> >> using n::i; >> using n::f; >> >> int var; >> extern int newvar __attribute__((alias("var"))); >> >> int main(){ >> i = 0; >> return 0; >> } >> > > Imported declaration is emitted for namespace alias. If you declare as below > in the above program: > > namespace m = n; > > Clang will generate, > > <1><23>: Abbrev Number: 2 (DW_TAG_namespace) > <24> DW_AT_name : (indexed string: 0x3): n > > [...] > > <1><59>: Abbrev Number: 7 (DW_TAG_imported_declaration) > <5a> DW_AT_decl_file : 0 > <5b> DW_AT_decl_line : 7 > <5c> DW_AT_import : <0x23> [Abbrev Number: 2 (DW_TAG_namespace)] > <60> DW_AT_name : (indexed string: 0xa): m > > This is handled in the "else" path. Ah, I didn't even know this was possible! Thanks for enlightening me. > >>> --- a/gdb/testsuite/gdb.base/symbol-alias.exp >>> +++ b/gdb/testsuite/gdb.base/symbol-alias.exp > >> Since this is a new feature of clang and some tests run on quite old systems, >> could you try and detect the clang version, so you can have xfails for all >> aliases if the clang version doesn't emit this information? > > Fixed. As of now, the clang version is checked for 14-0-0 and above. Can modify based on the version > in which this fix goes in finally. It should give 5 XFAILs for clang versions without the fix and 1 XFAIL > with the fix. > I have also added one more test point when the location information is missing (g_def_var2_alias2). > > --------------- > When clang emits the dwarf information of global alias variable > as DW_TAG_imported_declaration, gdb does not handle it. GDB reads > this tag as C++/fortran imported declaration (type alias, namespace > alias and fortran module). Added support to handle this tag as > alias variable. > > This change fixes the failure in gdb.base/symbol-alias.exp testcase. > This testcase is also updated to test nested (recursive) alias. > > Reviewed by: Bruno Larsen GDB doesn't use R-b tags, I think. The latest one I could find was from 2007. > --- > gdb/dwarf2/read.c | 74 +++++++++++++++++-------- > gdb/testsuite/gdb.base/symbol-alias.exp | 57 ++++++++++++++++++- > gdb/testsuite/gdb.base/symbol-alias2.c | 16 ++++++ > 3 files changed, 121 insertions(+), 26 deletions(-) > > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index f9c942d91d3..6510dd94ac3 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -1132,6 +1132,10 @@ static void add_partial_subprogram (struct partial_die_info *pdi, > CORE_ADDR *lowpc, CORE_ADDR *highpc, > int need_pc, struct dwarf2_cu *cu); > > +static void var_decode_location (struct attribute *attr, > + struct symbol *sym, > + struct dwarf2_cu *cu); > + > static unsigned int peek_abbrev_code (bfd *, const gdb_byte *); > > static struct partial_die_info *load_partial_dies > @@ -1332,7 +1336,7 @@ static struct using_direct **using_directives (struct dwarf2_cu *cu); > > static void read_import_statement (struct die_info *die, struct dwarf2_cu *); > > -static int read_namespace_alias (struct die_info *die, struct dwarf2_cu *cu); > +static bool read_alias (struct die_info *die, struct dwarf2_cu *cu); > > static struct type *read_module_type (struct die_info *die, > struct dwarf2_cu *cu); > @@ -9741,10 +9745,14 @@ process_die (struct die_info *die, struct dwarf2_cu *cu) > read_module (die, cu); > break; > case DW_TAG_imported_declaration: > + /* If the imported declaration is for global variable alias, > + this flag is set to false in read_alias function. */ > cu->processing_has_namespace_info = true; > - if (read_namespace_alias (die, cu)) > + if (((cu->per_cu->lang == language_c) > + || (cu->per_cu->lang == language_cplus)) > + && read_alias (die, cu)) > break; > - /* The declaration is not a global namespace alias. */ > + /* The declaration is not a global namespace or variable alias. */ > /* Fall through. */ > case DW_TAG_imported_module: > cu->processing_has_namespace_info = true; > @@ -10212,18 +10220,20 @@ dwarf2_physname (const char *name, struct die_info *die, struct dwarf2_cu *cu) > return retval; > } > > -/* Inspect DIE in CU for a namespace alias. If one exists, record > - a new symbol for it. > +#define MAX_NESTED_IMPORTED_DECLARATIONS 100 > > - Returns 1 if a namespace alias was recorded, 0 otherwise. */ > +/* Inspect DIE in CU for a namespace alias or a variable > + with alias attribute. If one exists, record a new symbol > + for it. > > -static int > -read_namespace_alias (struct die_info *die, struct dwarf2_cu *cu) > + Returns 1 if an alias was recorded, 0 otherwise. */ This comment needs to be updated, now that read_alias returns bool. > + > +static bool > +read_alias (struct die_info *die, struct dwarf2_cu *cu) > { > struct attribute *attr; > > - /* If the die does not have a name, this is not a namespace > - alias. */ > + /* The die must have a name for namespace or variable alias. */ > attr = dwarf2_attr (die, DW_AT_name, cu); Oops, I didn't catch this on my last review. DWARF 5 manual, section 3.2.3 Imported (or Renamed) Declaration Entries, says: An imported declaration may also have a DW_AT_name attribute whose value is (...) If no name is present, then the name by which the entity is to be known is the same as the name of the entity being imported. So I think the check and the comment are incorrect (or the spec needs updating). I know this isn't something you added, but since you are changing this function, I think it could be a good time to update this. > if (attr != NULL) > { > @@ -10232,13 +10242,12 @@ read_namespace_alias (struct die_info *die, struct dwarf2_cu *cu) > struct dwarf2_cu *imported_cu = cu; > > /* If the compiler has nested DW_AT_imported_declaration DIEs, > - keep inspecting DIEs until we hit the underlying import. */ > -#define MAX_NESTED_IMPORTED_DECLARATIONS 100 > + keep inspecting DIEs until we hit the underlying import. */ > for (num = 0; num < MAX_NESTED_IMPORTED_DECLARATIONS; ++num) > { > attr = dwarf2_attr (d, DW_AT_import, cu); > if (attr == NULL) > - break; > + return false; > > d = follow_die_ref (d, attr, &imported_cu); > if (d->tag != DW_TAG_imported_declaration) > @@ -10249,26 +10258,43 @@ read_namespace_alias (struct die_info *die, struct dwarf2_cu *cu) > { > complaint (_("DIE at %s has too many recursively imported " > "declarations"), sect_offset_str (d->sect_off)); > - return 0; > + return false; > } > > if (attr != NULL) > { > struct type *type; > - sect_offset sect_off = attr->get_ref_die_offset (); > - > - type = get_die_type_at_offset (sect_off, cu->per_cu, cu->per_objfile); > - if (type != NULL && type->code () == TYPE_CODE_NAMESPACE) > + if (d->tag == DW_TAG_variable) > { > - /* This declaration is a global namespace alias. Add > - a symbol for it whose type is the aliased namespace. */ > - new_symbol (die, type, cu); > - return 1; > + /* This declaration is a global variable alias. Add > + a symbol for it whose type is same as aliased variable. */ > + type = die_type (d, imported_cu); > + struct symbol *sym = new_symbol(die, type, cu); Space before ( > + attr = dwarf2_attr (d, DW_AT_location, imported_cu); > + sym->set_aclass_index (LOC_UNRESOLVED); > + if (attr != nullptr) > + { > + var_decode_location (attr, sym, cu); > + } > + /* Reset the flag as it is not a namespace alias. */ > + cu->processing_has_namespace_info = false; > + return true; > + } > + else > + { > + sect_offset sect_off = attr->get_ref_die_offset (); > + type = get_die_type_at_offset (sect_off, cu->per_cu, cu->per_objfile); > + if (type != NULL && type->code () == TYPE_CODE_NAMESPACE) > + { > + /* This declaration is a global namespace alias. Add > + a symbol for it whose type is the aliased namespace. */ > + new_symbol (die, type, cu); > + return true; > + } > } > } > } > - > - return 0; > + return false; > } > > /* Return the using directives repository (global or local?) to use in the > diff --git a/gdb/testsuite/gdb.base/symbol-alias.exp b/gdb/testsuite/gdb.base/symbol-alias.exp > index 2b53cc31053..253a60d5403 100644 > --- a/gdb/testsuite/gdb.base/symbol-alias.exp > +++ b/gdb/testsuite/gdb.base/symbol-alias.exp > @@ -15,6 +15,7 @@ > # along with this program. If not, see . > > standard_testfile symbol-alias.c symbol-alias2.c > +set using_clang [test_compiler_info {clang-1[4-9]-*-*}] I think it would be better if you tested for a failing clang instead. Otherwise you can set an XFAIL for gcc, which could hide a real GDB bug (we don't do much testing with clang, unfortunately). > > if { [prepare_for_testing "failed to prepare" ${testfile} [list $srcfile $srcfile2]] } { > return -1 > @@ -31,6 +32,58 @@ foreach f {"func" "func_alias"} { > } > > # Variables. > -foreach v {"g_var_s" "g_var_s_alias"} { > - gdb_test "p $v" "= {field1 = 1, field2 = 2}" > +gdb_test "p g_var_s" "= {field1 = 1, field2 = 2}" > +foreach v {"g_var_s_alias" "g_var_s_alias2"} { > + gdb_test_multiple "p $v" "p $v" { > + -re " = {field1 = 1, field2 = 2}.*$gdb_prompt $" { > + pass "print alias of variable $v" > + } > + -re ".*has unknown type; cast it to its declared type.*$gdb_prompt $" { > + if { ! $using_clang } { > + xfail "print alias variable $v" > + } else { > + fail "print alias variable $v" > + } > + } > + } > +} > + > +# Static Variable. > +gdb_test "p g_var" " = 1" > +gdb_test_multiple "p g_var_alias" "p g_var_alias" { > + -re " = 1.*$gdb_prompt $" { > + pass "print alias of static variable" > + } > + -re ".*has unknown type; cast it to its declared type.*$gdb_prompt $" { > + if { ! $using_clang } { > + xfail "print alias of static variable" > + } else { > + fail "print alias of static variable" > + } > + } > +} > + > +# Deferred Variable. > +gdb_test "p g_def_var" " = 2" > +gdb_test_multiple "p g_def_var_alias" "p g_def_var_alias" { > + -re " = 2.*$gdb_prompt $" { > + pass "print alias of deferred variable" > + } > + -re ".*has unknown type; cast it to its declared type.*$gdb_prompt $" { > + xfail "print alias of deferred variable" Since GDB + GCC is passing this test, you should also set the xfail only with when using a failing clang. > + } > +} > + > +# Alias of deferred Variable alias. > +gdb_test_multiple "p g_def_var2_alias2" "p g_def_var2_alias2" { > + -re " = 3.*$gdb_prompt $" { > + pass "print alias of alias of deferred variable" > + } > + -re ".*has unknown type; cast it to its declared type.*$gdb_prompt $" { > + if { ! $using_clang } { > + xfail "print alias of alias of deferred variable" > + } else { > + fail "print alias of alias of deferred variable" > + } > + } > } > diff --git a/gdb/testsuite/gdb.base/symbol-alias2.c b/gdb/testsuite/gdb.base/symbol-alias2.c > index 34f4e121e25..41d4b6a53e5 100644 > --- a/gdb/testsuite/gdb.base/symbol-alias2.c > +++ b/gdb/testsuite/gdb.base/symbol-alias2.c > @@ -23,6 +23,8 @@ struct S > > struct S g_var_s = { 1, 2 }; > > +static int g_var = 1; > + > #ifdef __cplusplus > /* So that the alias attribute below work without having to figure out > this function's mangled name. */ > @@ -38,3 +40,17 @@ func (void) > struct S *func_alias (void) __attribute__ ((alias ("func"))); > > extern struct S g_var_s_alias __attribute__ ((alias ("g_var_s"))); > + > +extern struct S g_var_s_alias2 __attribute__ ((alias ("g_var_s_alias"))); > + > +extern int g_var_alias __attribute__ ((alias ("g_var"))); > + > +extern int g_def_var_alias __attribute__ ((alias ("g_def_var"))); > + > +int g_def_var = 2; > + > +extern int g_def_var2_alias __attribute__ ((alias ("g_def_var2"))); > + > +int g_def_var2 = 3; > + > +extern int g_def_var2_alias2 __attribute__ ((alias ("g_def_var2_alias"))); > -- > > Regards, > Kavitha -- Cheers! Bruno Larsen