From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (vps-42846194.vps.ovh.net [IPv6:2001:41d0:801:2000::2400]) by sourceware.org (Postfix) with ESMTPS id 6164D395B81D for ; Wed, 16 Nov 2022 16:14:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6164D395B81D Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=lancelotsix.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=lancelotsix.com Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086:0:92cc:dfff:fea4:7a81]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 29BD580079; Wed, 16 Nov 2022 16:14:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=lancelotsix.com; s=2021; t=1668615273; bh=tMeoNVUDDd6YtZyRxuiMg3whmoQnTr7Qbvx8A9EY338=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BpMOKSmTJH5pmBPddQ0FrMQYy/beGUV86qRekQT5GoZz8js9BElxVHHNWfW9ZmCND ZJ/QopSY3qT9J+4BMSvn7gTUhEc7HutTWL+9Q90uo6fkVBBF51e1P0wWztjhAcutG3 blkE0w5ZeC7LvAphGW40SnWimwbW/wmcmK4TSDz7s+fy4JlmqyBdHMwGYZ+vsGcYN8 zQ9VyF04uYNxWlnUWJOBlMl2HaYcWosKoggJDg2H6HoKUg2Do3ZvghSWEAHRVB4/aw qjroQz8lotWi1yfFf7zu8Ug2Tkwg0+Ll/tXFgZHFH8uBn+FyUAzvxweF/CJqCNEcRH DFm+zXiAMnwJQ== Date: Wed, 16 Nov 2022 16:14:10 +0000 From: Lancelot SIX To: Bruno Larsen Cc: gdb-patches@sourceware.org, tom@tromey.com Subject: Re: [PATCH v2 1/2] gdb/c++: validate 'using' directives based on the current line Message-ID: <20221116161410.r3qixjxnmbluq2gg@ubuntu.lan> References: <20221116141336.1160869-1-blarsen@redhat.com> <20221116141336.1160869-2-blarsen@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20221116141336.1160869-2-blarsen@redhat.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Wed, 16 Nov 2022 16:14:33 +0000 (UTC) X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS,TXREP 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: Hi Bruno, I have included comments inlined in the patch. On Wed, Nov 16, 2022 at 03:13:36PM +0100, Bruno Larsen via Gdb-patches wrote: > When asking GDB to print a variable from an imported namespace, we only > want to see variables imported in lines that the inferior has already > gone through, as is being tested last in gdb.cp/nsusing.exp. However > with the proposed change to gdb.cp/nsusing.exp, we get the following > failures: > > (gdb) PASS: gdb.cp/nsusing.exp: continue to breakpoint: marker10 stop > print x > $9 = 911 > (gdb) FAIL: gdb.cp/nsusing.exp: print x, before using statement > next > 15 y += x; > (gdb) PASS: gdb.cp/nsusing.exp: using namespace M > print x > $10 = 911 > (gdb) PASS: gdb.cp/nsusing.exp: print x, only using M > > Showing that the feature wasn't functioning properly, it just so > happened that gcc ordered the namespaces in a convenient way. > This happens because GDB doesn't take into account the line where the > "using namespace" directive is written. So long as it shows up in the > current scope, we assume it is valid. > > To fix this, add a new member to struct using_direct, that stores the > line where the directive was written, and a new function that informs if > the using directive is valid already. > > Unfortunately, due to a GCC bug, the failure still shows up. Compilers > that set the declaration line of the using directive correctly (such as > Clang) do not show such a bug, so the test includes an XFAIL for gcc > code. > > Finally, because the final test of gdb.cp/nsusing.exp has turned into > multiple that all would need XFAILs for older GCCs (<= 4.3), and that > GCC is very old, if it is detected, the test just exits early. > --- > gdb/cp-namespace.c | 15 ++++++++++++--- > gdb/dwarf2/read.c | 25 ++++++++++++++++++++++++- > gdb/namespace.c | 25 +++++++++++++++++++++++++ > gdb/namespace.h | 16 +++++++++++++++- > gdb/testsuite/gdb.cp/nsusing.cc | 3 ++- > gdb/testsuite/gdb.cp/nsusing.exp | 16 +++++++++++++--- > 6 files changed, 91 insertions(+), 9 deletions(-) > > diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c > index 634dab6ada0..6ecb29fb1ac 100644 > --- a/gdb/cp-namespace.c > +++ b/gdb/cp-namespace.c > @@ -93,10 +93,12 @@ cp_scan_for_anonymous_namespaces (struct buildsym_compunit *compunit, > /* We've found a component of the name that's an > anonymous namespace. So add symbols in it to the > namespace given by the previous component if there is > - one, or to the global namespace if there isn't. */ > + one, or to the global namespace if there isn't. > + The declared line of this using directive can be set > + to 0, this way it is always considered valid. */ > std::vector excludes; > add_using_directive (compunit->get_local_using_directives (), > - dest, src, NULL, NULL, excludes, > + dest, src, NULL, NULL, excludes, 0, > 1, &objfile->objfile_obstack); > } > /* The "+ 2" is for the "::". */ > @@ -392,16 +394,23 @@ cp_lookup_symbol_via_imports (const char *scope, > if (sym.symbol != NULL) > return sym; > > + /* Due to a GCC bug, we need to know the boundaries of the current block > + to know if a certain using directive is valid. */ > + symtab_and_line boundary_sal = find_pc_line (block->end () - 1, 0); > + > /* Go through the using directives. If any of them add new names to > the namespace we're searching in, see if we can find a match by > applying them. */ > - > for (current = block_using (block); > current != NULL; > current = current->next) > { > const char **excludep; > > + /* If the using directive was below the place we are stopped at, > + do not use this directive. */ > + if (!current->valid_line (boundary_sal.line)) > + continue; > len = strlen (current->import_dest); > directive_match = (search_parents > ? (startswith (scope, current->import_dest) > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index 60e120a9d76..68e3149a4bb 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -9299,6 +9299,26 @@ using_directives (struct dwarf2_cu *cu) > return cu->get_builder ()->get_local_using_directives (); > } > > +/* Read the DW_ATTR_decl_line attribute for the given DIE in the > + given CU. If the format is not recognized or the attribute is > + not present, set it to 0. */ > + > +static unsigned int > +read_decl_line (struct die_info *die, struct dwarf2_cu *cu) > +{ > + > + struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu); > + if (decl_line == nullptr) > + return 0; > + if (decl_line->form_is_constant ()) > + return decl_line->constant_value (0); This is probably me being pedantic here, but constant_value return a LONGEST (i.e. long on x86_64) while read_decl_line returns an unsigned int. I really do not expect any realistic scenario where a line number goes above UINT_MAX, but I can easily imagine a buggy producer giving a negative value which would end up trash after the cast to unsigned int. Should we check that "0 <= decl_line->constant_value (0) <= UINT_MAX" ? > + else if (decl_line->form_is_unsigned ()) I do not see when this case should be possible. The DW_AT_decl_line attribute is of class "constant" (so one of DW_FORM_data[1,2,4,8,16], DW_FORM_[s,u]data] or DW_FORM_implicit_const. The only case not covered by form_is_constant is DW_FORM_data16 and it is not covered by form_is_unsigned either. I do believe that this is more a problem in attribute::form_is_constant / attribute::constant_value. Of course, the problem is that attribute::constant_value signature does not allow to return a 128bits value, but this is a question out of scope of this patch. Calling form_is_unsigned can return true if the form is DW_FORM_ref_addr or DW_FORM_sec_offset which would not make much sense in my opinion. I think I would remove this "else if" block completely as getting there would imply invalid DWARF. In such situation, I think returning 0 would the right thing to do. Best, Lancelot. > + return decl_line->as_unsigned (); > + > + complaint (_("Declared line for using directive is of incorrect format")); > + return 0; > +} > + > /* Read the import statement specified by the given die and record it. */ > > static void > @@ -9441,6 +9461,7 @@ read_import_statement (struct die_info *die, struct dwarf2_cu *cu) > import_alias, > imported_declaration, > excludes, > + read_decl_line (die, cu), > 0, > &objfile->objfile_obstack); > } > @@ -16078,7 +16099,9 @@ read_namespace (struct die_info *die, struct dwarf2_cu *cu) > std::vector excludes; > add_using_directive (using_directives (cu), > previous_prefix, type->name (), NULL, > - NULL, excludes, 0, &objfile->objfile_obstack); > + NULL, excludes, > + read_decl_line (die, cu), > + 0, &objfile->objfile_obstack); > } > } > > diff --git a/gdb/namespace.c b/gdb/namespace.c > index 0c39c921a3e..b2cca5a1da4 100644 > --- a/gdb/namespace.c > +++ b/gdb/namespace.c > @@ -18,6 +18,7 @@ > > #include "defs.h" > #include "namespace.h" > +#include "frame.h" > > /* Add a using directive to USING_DIRECTIVES. If the using directive > in question has already been added, don't add it twice. > @@ -40,6 +41,7 @@ add_using_directive (struct using_direct **using_directives, > const char *alias, > const char *declaration, > const std::vector &excludes, > + unsigned int decl_line, > int copy_names, > struct obstack *obstack) > { > @@ -76,6 +78,9 @@ add_using_directive (struct using_direct **using_directives, > if (ix < excludes.size () || current->excludes[ix] != NULL) > continue; > > + if (decl_line != current->decl_line) > + continue; > + > /* Parameters exactly match CURRENT. */ > return; > } > @@ -111,6 +116,26 @@ add_using_directive (struct using_direct **using_directives, > excludes.size () * sizeof (*newobj->excludes)); > newobj->excludes[excludes.size ()] = NULL; > > + newobj->decl_line = decl_line; > + > newobj->next = *using_directives; > *using_directives = newobj; > } > + > +/* See namespace.h. */ > + > +bool > +using_direct::valid_line (unsigned int boundary) const > +{ > + try > + { > + CORE_ADDR curr_pc = get_frame_pc (get_selected_frame (nullptr)); > + symtab_and_line curr_sal = find_pc_line (curr_pc, 0); > + return (decl_line <= curr_sal.line) > + || (decl_line >= boundary); > + } > + catch (const gdb_exception &ex) > + { > + return true; > + } > +} > diff --git a/gdb/namespace.h b/gdb/namespace.h > index dc052a44e42..b46806684c8 100644 > --- a/gdb/namespace.h > +++ b/gdb/namespace.h > @@ -30,7 +30,8 @@ > string representing the alias. Otherwise, ALIAS is NULL. > DECLARATION is the name of the imported declaration, if this import > statement represents one. Otherwise DECLARATION is NULL and this > - import statement represents a namespace. > + import statement represents a namespace. DECL_LINE is the line > + where the using directive is written in the source code. > > C++: using namespace A; > Fortran: use A > @@ -96,6 +97,11 @@ struct using_direct > > struct using_direct *next; > > + /* The line where the using directive was declared on the source file. > + This is used to check if the using directive is already active at the > + point where the inferior is stopped. */ > + unsigned int decl_line; > + > /* Used during import search to temporarily mark this node as > searched. */ > int searched; > @@ -103,6 +109,13 @@ struct using_direct > /* USING_DIRECT has variable allocation size according to the number of > EXCLUDES entries, the last entry is NULL. */ > const char *excludes[1]; > + > + /* Returns true if the using_direcive USING_DIR is valid in CURR_LINE. > + Because current GCC (at least version 12.2) sets the decl_line as > + the last line in the current block, we need to take this into > + consideration when checking the validity, by comparing it to > + BOUNDARY, the last line of the current block. */ > + bool valid_line (unsigned int boundary) const; > }; > > extern void add_using_directive (struct using_direct **using_directives, > @@ -111,6 +124,7 @@ extern void add_using_directive (struct using_direct **using_directives, > const char *alias, > const char *declaration, > const std::vector &excludes, > + const unsigned int decl_line, > int copy_names, > struct obstack *obstack); > > diff --git a/gdb/testsuite/gdb.cp/nsusing.cc b/gdb/testsuite/gdb.cp/nsusing.cc > index fa5c9d01f59..dcf0ba99e22 100644 > --- a/gdb/testsuite/gdb.cp/nsusing.cc > +++ b/gdb/testsuite/gdb.cp/nsusing.cc > @@ -10,8 +10,9 @@ namespace N > > int marker10 () > { > + int y = 1; // marker10 stop > using namespace M; > - int y = x + 1; // marker10 stop > + y += x; > using namespace N; > return y; > } > diff --git a/gdb/testsuite/gdb.cp/nsusing.exp b/gdb/testsuite/gdb.cp/nsusing.exp > index 2835207a21e..b79f3d26084 100644 > --- a/gdb/testsuite/gdb.cp/nsusing.exp > +++ b/gdb/testsuite/gdb.cp/nsusing.exp > @@ -120,8 +120,18 @@ gdb_continue_to_breakpoint "marker10 stop" > > if { [test_compiler_info {gcc-[0-3]-*}] || > [test_compiler_info {gcc-4-[0-3]-*}]} { > - setup_xfail *-*-* > + return > } > > -# Assert that M::x is printed and not N::x > -gdb_test "print x" "= 911" "print x (from M::x)" > +gdb_test_multiple "print x" "print x, before using statement" { > + -re -wrap "No symbol .x. in current context.*" { > + pass $gdb_test_name > + } > + -re -wrap "= 911.*" { > + # GCC doesn't properly set the decl_line for namespaces, so GDB believes > + # that the "using namespace M" line has already passed at this point. > + xfail $gdb_test_name > + } > +} > +gdb_test "next" ".*" "using namespace M" > +gdb_test "print x" "= 911" "print x, only using M" > -- > 2.38.1 >