From: Bruno Larsen <blarsen@redhat.com>
To: Lancelot SIX <lsix@lancelotsix.com>
Cc: gdb-patches@sourceware.org, tom@tromey.com
Subject: Re: [PATCH v2 1/2] gdb/c++: validate 'using' directives based on the current line
Date: Thu, 17 Nov 2022 10:12:34 +0100 [thread overview]
Message-ID: <a94a9f9d-eb82-4b15-6d99-4e9ea8369fb3@redhat.com> (raw)
In-Reply-To: <20221116161410.r3qixjxnmbluq2gg@ubuntu.lan>
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<const char *> 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<const char *> 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<const char *> &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<const char *> &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
>>
next prev parent reply other threads:[~2022-11-17 9:12 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-16 14:13 [PATCH v2 0/2] Improve handling of using directives Bruno Larsen
2022-11-16 14:13 ` [PATCH v2 1/2] gdb/c++: validate 'using' directives based on the current line Bruno Larsen
2022-11-16 16:14 ` Lancelot SIX
2022-11-17 9:12 ` Bruno Larsen [this message]
2022-11-16 14:13 ` [PATCH v2 2/2] gdb/c++: Detect ambiguous variables in imported namespaces Bruno Larsen
2022-11-16 17:49 ` Lancelot SIX
2022-11-17 9:58 ` Bruno Larsen
2022-11-17 22:07 ` Lancelot SIX
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a94a9f9d-eb82-4b15-6d99-4e9ea8369fb3@redhat.com \
--to=blarsen@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=lsix@lancelotsix.com \
--cc=tom@tromey.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).