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 D0FF23858D35 for ; Fri, 11 Nov 2022 14:44:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D0FF23858D35 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=1668177898; 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: in-reply-to:in-reply-to:references:references; bh=xMEu5hwftIp5u0rgyx6acbhgJy5p5kQ5H35u66YTEiM=; b=FewgJQZvm4vDJtPC027GP98h2MimEUfcN2aBy5kTDWBNS7SIBNbAn3mFi4/nc9fLlE61jN DT8MZww/5kuxCvTKkAxOSmTWyMv0jl92EPrHE8gt/Nbje0F6WEciRCYgawzMyKHAruJDi1 mDJzPDlVuCRGSSjQH6krizhfgyJnxrU= Received: from mail-qk1-f199.google.com (mail-qk1-f199.google.com [209.85.222.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-216-bCF7MPYgP4SidYyfKfbkHQ-1; Fri, 11 Nov 2022 09:44:57 -0500 X-MC-Unique: bCF7MPYgP4SidYyfKfbkHQ-1 Received: by mail-qk1-f199.google.com with SMTP id bk30-20020a05620a1a1e00b006fb2378c857so4080567qkb.18 for ; Fri, 11 Nov 2022 06:44:57 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=xMEu5hwftIp5u0rgyx6acbhgJy5p5kQ5H35u66YTEiM=; b=6qo86RBbrdqQe1QjJ0A6hcqGOLyPclL9KYkGzzPvZ67E7dwi6JBNghFH+fdgixLLIz a4jX7o3wTekC7aRut6HJRdphYuLradDuTwYhqrf9splA5pm/R0Lk1q7SeiuKlAzvB0Oa JO807kwQcCB+qjPf2cLgOCVoEerFTXecEvtxdUjNDgteYVDmP9OgPjDNuA5tecmJ6Sfz krSS2Pns4B334Gaa0KqFMZWqnr5WjJltcrQAuhDURO3AeifB8eV8jY5i8nzLo8EAoOKH 2r06NAIzLY6PkUdJ9fsTZT2e8Z6JWVzV0fpZyWq1afafBR2TI7lhxBEGZaJJ7+Kxy76t 2X6w== X-Gm-Message-State: ANoB5pmETFpuViAqPMzt0+/eZ7LSqDd1Nr8794i0tnVF4FdcIBfv3vbs IjNQno/3DX6mhWtJ0djTls4hgjngFCkwEEbecL5iYmMX2iJ7i4JWjexwfz672Ww9st++6m25+mZ 3ipSgV7hlfPwrZDfwo0UslKmEf+tS5RelVRJMqhDsXoKv3hVkFqCfzCj1+09OEVQhtsvDeaiIiA == X-Received: by 2002:a05:622a:4c0f:b0:3a2:9fcd:59c8 with SMTP id ey15-20020a05622a4c0f00b003a29fcd59c8mr1535159qtb.273.1668177896661; Fri, 11 Nov 2022 06:44:56 -0800 (PST) X-Google-Smtp-Source: AA0mqf7FEYPw8crGZLcMyGq8Qr6hYIb79GKgNAvQWOt6Cr40iB3Q9wYfIs46KgsGAATYf790U72LkQ== X-Received: by 2002:a05:622a:4c0f:b0:3a2:9fcd:59c8 with SMTP id ey15-20020a05622a4c0f00b003a29fcd59c8mr1535119qtb.273.1668177896213; Fri, 11 Nov 2022 06:44:56 -0800 (PST) Received: from localhost ([31.111.84.238]) by smtp.gmail.com with ESMTPSA id s13-20020a05620a16ad00b006fb112f512csm1467117qkj.74.2022.11.11.06.44.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 11 Nov 2022 06:44:55 -0800 (PST) From: Andrew Burgess To: Bruno Larsen via Gdb-patches , gdb-patches@sourceware.org Cc: Bruno Larsen Subject: Re: [PATCH 1/2] gdb/c++: validate 'using' directives based on the current line In-Reply-To: <20221026155053.3394792-2-blarsen@redhat.com> References: <20221026155053.3394792-1-blarsen@redhat.com> <20221026155053.3394792-2-blarsen@redhat.com> Date: Fri, 11 Nov 2022 14:44:53 +0000 Message-ID: <87mt8xa6u2.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,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: Bruno Larsen via Gdb-patches writes: > 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 | 30 +++++++++++++++++++++++++++++- > gdb/namespace.c | 26 ++++++++++++++++++++++++++ > gdb/namespace.h | 14 +++++++++++++- > gdb/testsuite/gdb.cp/nsusing.cc | 3 ++- > gdb/testsuite/gdb.cp/nsusing.exp | 16 +++++++++++++--- > 6 files changed, 95 insertions(+), 9 deletions(-) > > diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c > index 634dab6ada0..c1b6978b7c8 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 (!valid_line (current, 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 071d0c48e99..7755f87f5bb 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -9435,12 +9435,28 @@ read_import_statement (struct die_info *die, struct dwarf2_cu *cu) > process_die (child_die, cu); > } > > + /* When was the using directive was declared. > + If this was not supplied, set it to 0 so the directive is always valid. > + Since the type changes from DWARF 4 to DWARF 5, we must check > + the type of the attribute. */ > + struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu); > + unsigned int line; > + if (decl_line == nullptr) > + line = 0; > + else if (decl_line->form_is_constant ()) > + line = decl_line->constant_value (0); > + else if (decl_line->form_is_unsigned ()) > + line = decl_line->as_unsigned (); > + else > + gdb_assert_not_reached (); > + > add_using_directive (using_directives (cu), > import_prefix, > canonical_name, > import_alias, > imported_declaration, > excludes, > + line, > 0, > &objfile->objfile_obstack); > } > @@ -16064,9 +16080,21 @@ read_namespace (struct die_info *die, struct dwarf2_cu *cu) > const char *previous_prefix = determine_prefix (die, cu); > > std::vector excludes; > + struct attribute *decl_line = dwarf2_attr (die, DW_AT_decl_line, cu); > + unsigned int line; > + if (decl_line == nullptr) > + line = 0; > + else if (decl_line->form_is_constant ()) > + line = decl_line->constant_value (0); > + else if (decl_line->form_is_unsigned ()) > + line = decl_line->as_unsigned (); > + else > + gdb_assert_not_reached (); > add_using_directive (using_directives (cu), > previous_prefix, type->name (), NULL, > - NULL, excludes, 0, &objfile->objfile_obstack); > + NULL, excludes, > + line, > + 0, &objfile->objfile_obstack); > } > } > > diff --git a/gdb/namespace.c b/gdb/namespace.c > index 0c39c921a3e..d467b0c80bb 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,27 @@ 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 > +valid_line (struct using_direct *using_dir, > + unsigned int boundary) > +{ > + try > + { > + CORE_ADDR curr_pc = get_frame_pc (get_selected_frame (nullptr)); > + symtab_and_line curr_sal = find_pc_line (curr_pc, 0); > + return using_dir->decl_line <= curr_sal.line > + || using_dir->decl_line >= boundary; > + } > + catch (const gdb_exception &ex) > + { > + return true; > + } > +} > diff --git a/gdb/namespace.h b/gdb/namespace.h > index dc052a44e42..ed97f9cf804 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,8 @@ struct using_direct > > struct using_direct *next; > > + unsigned int decl_line; > + > /* Used during import search to temporarily mark this node as > searched. */ > int searched; > @@ -111,7 +114,16 @@ 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); > > +/* 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. */ > +extern bool valid_line (struct using_direct *using_dir, > + unsigned int boundary); > + > #endif /* NAMESPACE_H */ > 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..bce6e30adc1 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 > + } > + # 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. I'm pretty sure that comments at this scope will mess up the gdb_test_multiple. I don't recall the details, but it's something to do with the way we expand the blocks to build the final gdb_expect call. I think you need to move the comment to... > + -re -wrap "= 911.*" { ... here. Thanks, Andrew > + xfail $gdb_test_name > + } > +} > +gdb_test "next" ".*" "using namespace M" > +gdb_test "print x" "= 911" "print x, only using M" > -- > 2.37.3