From: Andrew Burgess <aburgess@redhat.com>
To: Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org>,
gdb-patches@sourceware.org
Cc: Bruno Larsen <blarsen@redhat.com>
Subject: Re: [PATCH 1/2] gdb/c++: validate 'using' directives based on the current line
Date: Fri, 11 Nov 2022 14:44:53 +0000 [thread overview]
Message-ID: <87mt8xa6u2.fsf@redhat.com> (raw)
In-Reply-To: <20221026155053.3394792-2-blarsen@redhat.com>
Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> 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<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 (!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<const char *> 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<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,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<const char *> &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
next prev parent reply other threads:[~2022-11-11 14:44 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-26 15:50 [PATCH 0/2] Improve handling of using directives Bruno Larsen
2022-10-26 15:50 ` [PATCH 1/2] gdb/c++: validate 'using' directives based on the current line Bruno Larsen
2022-11-09 17:03 ` Tom Tromey
2022-11-11 14:44 ` Andrew Burgess [this message]
2022-10-26 15:50 ` [PATCH 2/2] gdb/c++: Detect ambiguous variables in imported namespaces Bruno Larsen
2022-11-09 17:11 ` Tom Tromey
2022-11-11 15:34 ` Bruno Larsen
2022-11-11 16:37 ` Tom Tromey
2022-11-11 14:30 ` Andrew Burgess
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=87mt8xa6u2.fsf@redhat.com \
--to=aburgess@redhat.com \
--cc=blarsen@redhat.com \
--cc=gdb-patches@sourceware.org \
/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).