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 43D2D38493ED for ; Thu, 17 Nov 2022 09:12:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 43D2D38493ED 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=1668676357; 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=Pa6Zvv/75uG5ET2V74uc2FthQdIDa4+B9I1Ey5q9Ijw=; b=gizk7wNyykZPftGyADKIjFw9BDzyuLh6JWDeG9po5TMkkKxE/k0qQZn7DjdtLC2n12t5ZS xP/lHioypMmC8RzeM1cgmI8xhNKd+jKxh2H7K5jq10lKJNnelin1Vw0b7ds3+lBsmMfqy/ +G+SEY+baLs35wqU0sNhz35SGTU51XQ= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-558-gwY1qiueNJSJeAf3S4RYRQ-1; Thu, 17 Nov 2022 04:12:36 -0500 X-MC-Unique: gwY1qiueNJSJeAf3S4RYRQ-1 Received: by mail-wm1-f72.google.com with SMTP id j2-20020a05600c1c0200b003cf7397fc9bso361459wms.5 for ; Thu, 17 Nov 2022 01:12:36 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Pa6Zvv/75uG5ET2V74uc2FthQdIDa4+B9I1Ey5q9Ijw=; b=wPh44rXEBZkZ3IzA80g/qjyKrdg6OU602VRLSu3pM4AH+JhIAD5epX0khEHXvAodpB cGt9bK2I3ln00CqKN1VCcdzw/OdbJQaFRaXx3ahTKF5YpdRgx+tJEb4IHCZKeiW4op8S FK3qChcidEYPpmmhzVs0tgfTCLJkZl2C4WeWC+Y167ReK1oJHsQ9afMsk2IiPObOK7ap 8WFzVWMnLspBbgCN0/eVegJkG7R2JCzUVZiD6+sRK7w1/TI4YHQxNkBXyohCodZ5oS9F WWdRZhhSAvgfxbiwDBVn1EMulVNyavh3J4dzdRLDryOvIG3z7k9dvUpHKXxkAmNpoV1X kmxg== X-Gm-Message-State: ANoB5pn4Nkc7evKRiGcuXsbH4MRhuRyU6MVHbKkm0YRh4f2ey1ZdKAqp jVBRQPc0b7ZTWsYzaTBB+/kQQO18/tuq0vhkpyLR4RfmSCF9lTBkaSA4HfqfjrXR96BHFD2+3gn r7tqPR8UH7jJSqqSFkEiA6w== X-Received: by 2002:a5d:50c1:0:b0:236:4c77:7d38 with SMTP id f1-20020a5d50c1000000b002364c777d38mr921452wrt.7.1668676355460; Thu, 17 Nov 2022 01:12:35 -0800 (PST) X-Google-Smtp-Source: AA0mqf4+KQwW3VXiI5raISRM1JOMjLrBMh7x8Cep+Jiy1ax0KVTXAZPCgPZzupbEI4f1fhIroWM9aw== X-Received: by 2002:a5d:50c1:0:b0:236:4c77:7d38 with SMTP id f1-20020a5d50c1000000b002364c777d38mr921431wrt.7.1668676355061; Thu, 17 Nov 2022 01:12:35 -0800 (PST) Received: from [192.168.0.45] (ip-62-245-66-121.bb.vodafone.cz. [62.245.66.121]) by smtp.gmail.com with ESMTPSA id r13-20020a05600c458d00b003c7087f6c9asm5187447wmo.32.2022.11.17.01.12.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 17 Nov 2022 01:12:34 -0800 (PST) Message-ID: Date: Thu, 17 Nov 2022 10:12:34 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.1 Subject: Re: [PATCH v2 1/2] gdb/c++: validate 'using' directives based on the current line To: Lancelot SIX Cc: gdb-patches@sourceware.org, tom@tromey.com References: <20221116141336.1160869-1-blarsen@redhat.com> <20221116141336.1160869-2-blarsen@redhat.com> <20221116161410.r3qixjxnmbluq2gg@ubuntu.lan> From: Bruno Larsen In-Reply-To: <20221116161410.r3qixjxnmbluq2gg@ubuntu.lan> 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=-10.3 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_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: On 16/11/2022 17:14, Lancelot SIX wrote: > 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" ? Sure, I could add a check and complaint here. > >> + 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. Well, now I'm very confused... When I was just starting out, I needed this to make the patch work with clang, but now it doesn't seem necessary anymore. Thanks for double checking it! -- Cheers, Bruno > > 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 >>