On Mon, Jul 08, 2024 at 02:24:14PM -0400, Jason Merrill wrote:
> On 7/6/24 10:13 PM, Nathaniel Shead wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> >
> > I have also been working on a patch that uses the locations of
> > using-decls in 'diagnose_name_conflict' and 'duplicate_decls' calls, but
> > that will need a fair bit more work that I'll probaby put off for the
> > meantime. Otherwise, as far as I can tell there's no circumstance when
> > modules-imported namespace-scope USING_DECLs have their locations
> > printed (hence the lack of tests), so I'm happy to defer this patch
> > until I've made something I can actually test.
> >
> > An alternative approach here (rather than giving locations to OVERLOADs)
> > would be to rewrite the modules handling to not using OVERLOADs
> > internally for non-function usings, that way we can attach the location
> > to those USING_DECLs. I went this way because I felt having locations
> > in OVL_USING_P overloads would be useful anyway, but happy to try that
> > route instead if you're uncomfortable increasing the size of most
> > OVERLOADs for something largely unused.
>
> That sounds better to me.
OK. Just for some additional context, the diagnostics I was thinking of
were in regards to PR c++/106851; currently the 'conflicts with' for
such declarations just point to an apparently unrelated declaration in a
different namespace, and provides no indication of the using-declaration
that brought it into scope. Ideally I'd like to add another "note: brought
into scope here" message for all such conflicts with using-decls, but
that means that we need to actually track this for function using-decls
too.
That said, I imagine we could do this by instead of having a flag on the
OVERLOAD, instead have the OVERLOAD wrap a USING_DECL that has this
location on it. Which may be slightly less confusing, and should also
optimise for the (I expect?) common case of overloads not being usings.
Nathaniel
>
> > -- >8 --
> >
> > This is used by module streaming to track locations of USING_DECLs (that
> > are internally wrapped and unwrapped as OVERLOADs for consistency with
> > function usings). No testcases with this patch as there aren't any easy
> > ways to actually cause a diagnostic that uses this information yet;
> > that'll come in a later patch.
> >
> > gcc/cp/ChangeLog:
> >
> > * cp-tree.h (OVL_SOURCE_LOCATION): New.
> > (struct tree_overload): Add loc field.
> > (class ovl_iterator): New member function.
> > * module.cc (depset::hash::add_binding_entity): Add parameter.
> > Track locations of usings.
> > (depset::hash::find_dependencies): Note locations of usings.
> > (module_state::write_cluster): Write locations of usings.
> > (module_state::read_cluster): Read locations of usings.
> > * name-lookup.cc (ovl_iterator::source_location): New.
> > (walk_module_binding): Add parameter to callback. Provide
> > locations of usings.
> > * name-lookup.h (walk_module_binding): Add parameter to
> > callback.
> > * ptree.cc (cxx_print_xnode): Write using location.
> > * tree.cc (ovl_insert): Initialise source location for usings.
> > (lookup_maybe_add): Propagate source location for usings.
> >
> > Signed-off-by: Nathaniel Shead <nathanielosh...@gmail.com>
> > ---
> > gcc/cp/cp-tree.h | 6 ++++++
> > gcc/cp/module.cc | 29 +++++++++++++++++++----------
> > gcc/cp/name-lookup.cc | 32 ++++++++++++++++++++++++++------
> > gcc/cp/name-lookup.h | 3 ++-
> > gcc/cp/ptree.cc | 6 ++++++
> > gcc/cp/tree.cc | 2 ++
> > 6 files changed, 61 insertions(+), 17 deletions(-)
> >
> > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > index 8732b7dc71b..cc7c1947f9e 100644
> > --- a/gcc/cp/cp-tree.h
> > +++ b/gcc/cp/cp-tree.h
> > @@ -824,6 +824,10 @@ typedef struct ptrmem_cst * ptrmem_cst_t;
> > /* The name of the overload set. */
> > #define OVL_NAME(NODE) DECL_NAME (OVL_FIRST (NODE))
> > +/* The source location of this OVL_USING_P overload. */
> > +#define OVL_SOURCE_LOCATION(NODE) \
> > + (((struct tree_overload*)OVERLOAD_CHECK (NODE))->loc)
> > +
> > /* Whether this is a set of overloaded functions. TEMPLATE_DECLS are
> > always wrapped in an OVERLOAD, so we don't need to check them
> > here. */
> > @@ -838,6 +842,7 @@ typedef struct ptrmem_cst * ptrmem_cst_t;
> > struct GTY(()) tree_overload {
> > struct tree_common common;
> > tree function;
> > + location_t loc;
> > };
> > /* Iterator for a 1 dimensional overload. Permits iterating over the
> > @@ -895,6 +900,7 @@ class ovl_iterator {
> > }
> > bool purview_p () const;
> > bool exporting_p () const;
> > + location_t source_location () const;
> > public:
> > tree remove_node (tree head)
> > diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> > index f5966fc8c1c..5bb3e824acb 100644
> > --- a/gcc/cp/module.cc
> > +++ b/gcc/cp/module.cc
> > @@ -2581,7 +2581,7 @@ public:
> > void add_namespace_context (depset *, tree ns);
> > private:
> > - static bool add_binding_entity (tree, WMB_Flags, void *);
> > + static bool add_binding_entity (tree, WMB_Flags, void *, location_t);
> > public:
> > bool add_namespace_entities (tree ns, bitmap partitions);
> > @@ -13122,7 +13122,8 @@ struct add_binding_data
> > /* Return true if we are, or contain something that is exported. */
> > bool
> > -depset::hash::add_binding_entity (tree decl, WMB_Flags flags, void *data_)
> > +depset::hash::add_binding_entity (tree decl, WMB_Flags flags, void *data_,
> > + location_t loc)
> > {
> > auto data = static_cast <add_binding_data *> (data_);
> > decl = strip_using_decl (decl);
> > @@ -13185,7 +13186,6 @@ depset::hash::add_binding_entity (tree decl,
> > WMB_Flags flags, void *data_)
> > {
> > if (!(flags & WMB_Hidden))
> > d->clear_hidden_binding ();
> > - OVL_PURVIEW_P (d->get_entity ()) = true;
> > if (flags & WMB_Export)
> > OVL_EXPORT_P (d->get_entity ()) = true;
> > return bool (flags & WMB_Export);
> > @@ -13229,6 +13229,7 @@ depset::hash::add_binding_entity (tree decl,
> > WMB_Flags flags, void *data_)
> > OVL_PURVIEW_P (decl) = true;
> > if (flags & WMB_Export)
> > OVL_EXPORT_P (decl) = true;
> > + OVL_SOURCE_LOCATION (decl) = loc;
> > }
> > depset *dep = data->hash->make_dependency
> > @@ -13616,7 +13617,10 @@ depset::hash::find_dependencies (module_state
> > *module)
> > dump.indent ();
> > walker.begin ();
> > if (current->get_entity_kind () == EK_USING)
> > - walker.tree_node (OVL_FUNCTION (decl));
> > + {
> > + module->note_location (OVL_SOURCE_LOCATION (decl));
> > + walker.tree_node (OVL_FUNCTION (decl));
> > + }
> > else if (TREE_VISITED (decl))
> > /* A global tree. */;
> > else if (item->get_entity_kind () == EK_NAMESPACE)
> > @@ -15114,6 +15118,7 @@ module_state::write_cluster (elf_out *to, depset
> > *scc[], unsigned size,
> > depset *dep = b->deps[jx];
> > tree bound = dep->get_entity ();
> > unsigned flags = 0;
> > + location_t loc = UNKNOWN_LOCATION;
> > if (dep->get_entity_kind () == depset::EK_USING)
> > {
> > tree ovl = bound;
> > @@ -15126,6 +15131,7 @@ module_state::write_cluster (elf_out *to, depset
> > *scc[], unsigned size,
> > flags |= cbf_using;
> > if (OVL_EXPORT_P (ovl))
> > flags |= cbf_export;
> > + loc = OVL_SOURCE_LOCATION (ovl);
> > }
> > else
> > {
> > @@ -15140,6 +15146,8 @@ module_state::write_cluster (elf_out *to, depset
> > *scc[], unsigned size,
> > gcc_checking_assert (DECL_P (bound));
> > sec.i (flags);
> > + if (flags & cbf_using)
> > + write_location (sec, loc);
> > sec.tree_node (bound);
> > }
> > @@ -15281,6 +15289,10 @@ module_state::read_cluster (unsigned snum)
> > && (flags & (cbf_using | cbf_export)))
> > sec.set_overrun ();
> > + location_t loc = UNKNOWN_LOCATION;
> > + if (flags & cbf_using)
> > + loc = read_location (sec);
> > +
> > tree decl = sec.tree_node ();
> > if (sec.get_overrun ())
> > break;
> > @@ -15291,8 +15303,7 @@ module_state::read_cluster (unsigned snum)
> > if (type || !DECL_IMPLICIT_TYPEDEF_P (decl))
> > sec.set_overrun ();
> > - type = build_lang_decl_loc (UNKNOWN_LOCATION,
> > - USING_DECL,
> > + type = build_lang_decl_loc (loc, USING_DECL,
> > DECL_NAME (decl),
> > NULL_TREE);
> > USING_DECL_DECLS (type) = decl;
> > @@ -15313,10 +15324,7 @@ module_state::read_cluster (unsigned snum)
> > if (decls)
> > sec.set_overrun ();
> > - /* FIXME: Propagate the location of the using-decl
> > - for use in diagnostics. */
> > - decls = build_lang_decl_loc (UNKNOWN_LOCATION,
> > - USING_DECL,
> > + decls = build_lang_decl_loc (loc, USING_DECL,
> > DECL_NAME (decl),
> > NULL_TREE);
> > USING_DECL_DECLS (decls) = decl;
> > @@ -15339,6 +15347,7 @@ module_state::read_cluster (unsigned snum)
> > OVL_PURVIEW_P (decls) = true;
> > if (flags & cbf_export)
> > OVL_EXPORT_P (decls) = true;
> > + OVL_SOURCE_LOCATION (decls) = loc;
> > }
> > if (flags & cbf_hidden)
> > diff --git a/gcc/cp/name-lookup.cc b/gcc/cp/name-lookup.cc
> > index 96d7f938162..52c07e46d4f 100644
> > --- a/gcc/cp/name-lookup.cc
> > +++ b/gcc/cp/name-lookup.cc
> > @@ -4229,6 +4229,17 @@ ovl_iterator::exporting_p () const
> > return OVL_EXPORT_P (ovl);
> > }
> > +/* The declared source location of this using. */
> > +
> > +location_t
> > +ovl_iterator::source_location () const
> > +{
> > + gcc_checking_assert (using_p ());
> > + if (TREE_CODE (ovl) == USING_DECL)
> > + return DECL_SOURCE_LOCATION (ovl);
> > + return OVL_SOURCE_LOCATION (ovl);
> > +}
> > +
> > /* Given a namespace-level binding BINDING, walk it, calling CALLBACK
> > for all decls of the current module. When partitions are involved,
> > decls might be mentioned more than once. Return the accumulation of
> > @@ -4236,7 +4247,8 @@ ovl_iterator::exporting_p () const
> > unsigned
> > walk_module_binding (tree binding, bitmap partitions,
> > - bool (*callback) (tree decl, WMB_Flags, void *data),
> > + bool (*callback) (tree decl, WMB_Flags, void *data,
> > + location_t loc),
> > void *data)
> > {
> > tree current = binding;
> > @@ -4249,6 +4261,7 @@ walk_module_binding (tree binding, bitmap partitions,
> > if (tree type = MAYBE_STAT_TYPE (current))
> > {
> > WMB_Flags flags = WMB_None;
> > + location_t loc = UNKNOWN_LOCATION;
> > if (STAT_TYPE_HIDDEN_P (current))
> > flags = WMB_Flags (flags | WMB_Hidden);
> > if (TREE_CODE (type) == USING_DECL)
> > @@ -4258,8 +4271,9 @@ walk_module_binding (tree binding, bitmap partitions,
> > flags = WMB_Flags (flags | WMB_Purview);
> > if (DECL_MODULE_EXPORT_P (type))
> > flags = WMB_Flags (flags | WMB_Export);
> > + loc = DECL_SOURCE_LOCATION (type);
> > }
> > - count += callback (type, flags, data);
> > + count += callback (type, flags, data, loc);
> > decl_hidden = STAT_DECL_HIDDEN_P (current);
> > }
> > @@ -4270,6 +4284,7 @@ walk_module_binding (tree binding, bitmap partitions,
> > if (!(decl_hidden && DECL_IS_UNDECLARED_BUILTIN (*iter)))
> > {
> > WMB_Flags flags = WMB_None;
> > + location_t loc = UNKNOWN_LOCATION;
> > if (decl_hidden)
> > flags = WMB_Flags (flags | WMB_Hidden);
> > if (iter.using_p ())
> > @@ -4279,8 +4294,9 @@ walk_module_binding (tree binding, bitmap partitions,
> > flags = WMB_Flags (flags | WMB_Purview);
> > if (iter.exporting_p ())
> > flags = WMB_Flags (flags | WMB_Export);
> > + loc = iter.source_location ();
> > }
> > - count += callback (*iter, flags, data);
> > + count += callback (*iter, flags, data, loc);
> > }
> > decl_hidden = false;
> > }
> > @@ -4319,13 +4335,14 @@ walk_module_binding (tree binding, bitmap
> > partitions,
> > WMB_Flags flags = WMB_None;
> > if (maybe_dups)
> > flags = WMB_Flags (flags | WMB_Dups);
> > - count += callback (bind, flags, data);
> > + count += callback (bind, flags, data, UNKNOWN_LOCATION);
> > }
> > else if (STAT_HACK_P (bind) && MODULE_BINDING_PARTITION_P
> > (bind))
> > {
> > if (tree btype = STAT_TYPE (bind))
> > {
> > WMB_Flags flags = WMB_None;
> > + location_t loc = UNKNOWN_LOCATION;
> > if (maybe_dups)
> > flags = WMB_Flags (flags | WMB_Dups);
> > if (STAT_TYPE_HIDDEN_P (bind))
> > @@ -4337,8 +4354,9 @@ walk_module_binding (tree binding, bitmap partitions,
> > flags = WMB_Flags (flags | WMB_Purview);
> > if (DECL_MODULE_EXPORT_P (btype))
> > flags = WMB_Flags (flags | WMB_Export);
> > + loc = DECL_SOURCE_LOCATION (btype);
> > }
> > - count += callback (btype, flags, data);
> > + count += callback (btype, flags, data, loc);
> > }
> > bool part_hidden = STAT_DECL_HIDDEN_P (bind);
> > for (ovl_iterator iter (MAYBE_STAT_DECL (STAT_DECL (bind)));
> > @@ -4350,6 +4368,7 @@ walk_module_binding (tree binding, bitmap partitions,
> > (!(part_hidden && DECL_IS_UNDECLARED_BUILTIN
> > (*iter)));
> > WMB_Flags flags = WMB_None;
> > + location_t loc = UNKNOWN_LOCATION;
> > if (maybe_dups)
> > flags = WMB_Flags (flags | WMB_Dups);
> > if (part_hidden)
> > @@ -4361,8 +4380,9 @@ walk_module_binding (tree binding, bitmap partitions,
> > flags = WMB_Flags (flags | WMB_Purview);
> > if (iter.exporting_p ())
> > flags = WMB_Flags (flags | WMB_Export);
> > + loc = iter.source_location ();
> > }
> > - count += callback (*iter, flags, data);
> > + count += callback (*iter, flags, data, loc);
> > part_hidden = false;
> > }
> > }
> > diff --git a/gcc/cp/name-lookup.h b/gcc/cp/name-lookup.h
> > index 5cf6ae6374a..8183cd50159 100644
> > --- a/gcc/cp/name-lookup.h
> > +++ b/gcc/cp/name-lookup.h
> > @@ -499,7 +499,8 @@ enum WMB_Flags
> > };
> > extern unsigned walk_module_binding (tree binding, bitmap partitions,
> > - bool (*)(tree decl, WMB_Flags, void *data),
> > + bool (*)(tree decl, WMB_Flags, void *data,
> > + location_t loc),
> > void *data);
> > extern tree add_imported_namespace (tree ctx, tree name, location_t,
> > unsigned module,
> > diff --git a/gcc/cp/ptree.cc b/gcc/cp/ptree.cc
> > index 15e46752d01..4f7fc96cc74 100644
> > --- a/gcc/cp/ptree.cc
> > +++ b/gcc/cp/ptree.cc
> > @@ -299,6 +299,12 @@ cxx_print_xnode (FILE *file, tree node, int indent)
> > print_node (file, "optype", BASELINK_OPTYPE (node), indent + 4);
> > break;
> > case OVERLOAD:
> > + if (location_t loc = OVL_SOURCE_LOCATION (node))
> > + {
> > + expanded_location xloc = expand_location (loc);
> > + indent_to (file, indent + 4);
> > + fprintf (file, "%s:%d:%d", xloc.file, xloc.line, xloc.column);
> > + }
> > print_node (file, "function", OVL_FUNCTION (node), indent + 4);
> > print_node (file, "next", OVL_CHAIN (node), indent + 4);
> > break;
> > diff --git a/gcc/cp/tree.cc b/gcc/cp/tree.cc
> > index dfd4a3a948b..21d8e0de329 100644
> > --- a/gcc/cp/tree.cc
> > +++ b/gcc/cp/tree.cc
> > @@ -2387,6 +2387,7 @@ ovl_insert (tree fn, tree maybe_ovl, int
> > using_or_hidden)
> > OVL_PURVIEW_P (maybe_ovl) = true;
> > if (using_or_hidden > 2)
> > OVL_EXPORT_P (maybe_ovl) = true;
> > + OVL_SOURCE_LOCATION (maybe_ovl) = input_location;
> > }
> > }
> > else
> > @@ -2532,6 +2533,7 @@ lookup_maybe_add (tree fns, tree lookup, bool
> > deduping)
> > {
> > lookup = ovl_make (OVL_FUNCTION (fns), lookup);
> > OVL_USING_P (lookup) = true;
> > + OVL_SOURCE_LOCATION (lookup) = OVL_SOURCE_LOCATION (fns);
> > }
> > else
> > lookup = lookup_add (OVL_FUNCTION (fns), lookup);
>
