public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Patrick Palka <ppalka@redhat.com>
To: Nathan Sidwell <nathan@acm.org>
Cc: Patrick Palka <ppalka@redhat.com>,
	gcc-patches@gcc.gnu.org,  jason@redhat.com
Subject: Re: [PATCH] c++ modules: ICE with class NTTP argument [PR100616]
Date: Wed, 28 Sep 2022 10:42:04 -0400 (EDT)	[thread overview]
Message-ID: <5943173d-4a56-c759-d1c2-80f389723ff1@idea> (raw)
In-Reply-To: <ec5fcb6f-82eb-58ec-0f23-68be70aad014@acm.org>

[-- Attachment #1: Type: text/plain, Size: 8247 bytes --]

On Tue, 27 Sep 2022, Nathan Sidwell wrote:

> On 9/26/22 15:05, Patrick Palka wrote:
> > On Mon, 26 Sep 2022, Patrick Palka wrote:
> > 
> > > On Mon, 26 Sep 2022, Nathan Sidwell wrote:
> > > 
> > > > On 9/26/22 10:08, Nathan Sidwell wrote:
> > > > > On 9/23/22 09:32, Patrick Palka wrote:
> > > > > 
> > > > > > Judging by the two commits that introduced/modified this part of
> > > > > > maybe_register_incomplete_var, r196852 and r214333, ISTM the code
> > > > > > is really only concerned with constexpr static data members (whose
> > > > > > initializer may contain a pointer-to-member for a currently open
> > > > > > class).
> > > > > > So maybe we ought to restrict the branch like so, which effectively
> > > > > > disables this part of maybe_register_incomplete_var during
> > > > > > stream-in, and
> > > > > > guarantees that outermost_open_class doesn't return NULL if the
> > > > > > branch is
> > > > > > taken?
> > > > > 
> > > > > I think the problem is that we're streaming these VAR_DECLs as regular
> > > > > VAR_DECLS, when we should be handling them as a new kind of object
> > > > > fished
> > > > > out from the template they're instantiating. (I'm guessing that'll
> > > > > just be a
> > > > > new tag, a type and an initializer?)
> > > > > 
> > > > > Then on stream-in we can handle them in the same way as a non-modules
> > > > > compilation handles such redeclarations.  I.e. how does:
> > > > > 
> > > > > template<auto> struct C { };
> > > > > struct A { };
> > > > > C<A{}> c1; // #1
> > > > > C<A{}> c2; // #2
> > > > > 
> > > > > work.  Presumably at some point #2's A{} gets unified such that we
> > > > > find the
> > > > > instantation that occurred at #1?
> > > 
> > > This works because the lookup in get_template_parm_object for #2's A{}
> > > finds and reuses the VAR_DECL created for #1's A{}.
> > > 
> > > But IIUC this lookup (performed via get_global_binding) isn't
> > > import-aware, which I suppose explains why we don't find the VAR_DECL
> > > from another TU.
> > > 
> > > > > 
> > > > > I notice the template arg for C<A{}> is a var decl mangled as
> > > > > _ZTAXtl1AEE,
> > > > > which is a 'template paramete object for A{}'.  I see that's a special
> > > > > mangler 'mangle_template_parm_object', called from
> > > > > get_template_parm_object.  Perhaps these VAR_DECLs need an additional
> > > > > in-tree flag that the streamer can check for?
> > > > 
> > > > I wonder if we're setting the module attachment for these variables
> > > > sanely?
> > > > They should be attached to the global module.  My guess is the
> > > > pushdecl_top_level_and_finish call in get_templatE_parm_object is not
> > > > doing
> > > > what is needed (as well as the other issues).
> > > 
> > > This is a bit of a shot in the dark, but the following seems to work:
> > > when pushing the VAR_DECL, we need to call set_originating_module to
> > > attach it to the global module, and when looking it up, we need to do so
> > > in an import-aware way.  Hopefully something like this is sufficient
> > > to properly handle these VAR_DECLs and we don't need to stream them
> > > specially?
> > 
> > Err, rather than changing the behavior of get_namespace_binding (which
> > has many unrelated callers), I guess we could just use the already
> > import-aware lookup_qualified_name instead where appropriate.  WDYT of
> > the following? (testing in progress)
> 
> I'm going to have to think further.  Morally these VAR_DECLs are like the
> typeinfo objects -- which we have to handle specially.  Reminding myself, I
> see rtti.cc does the pushdecl_top_level stuff creating them -- so they go into
> the slot for the current TU.  But the streaming code writes
> tt_tinfo_var/tt_tinfo_typedef records, and recreates the typeinfo on stream
> in, using the same above pushdec_top_level path. So even though the tinfo
> decls might seem attached to the current module, that doesn;t confuse the
> streaming, nor create collisions on read back.  Nor do we stream out tinfo
> decls that are not reachable through the streamed AST (if we treated then as
> normal decls, we'd stream them cos they're inthe current TU in the symbol
> table.  I have the same fear for these NTTPs.)
> 
> It looks like TREE_LANG_FLAG_5 can be used to note these VAR_DECLs are NTTPs,
> and then the streaming can deal with them.  Let me look further.

I see, thanks very much for the enlightening explanation.

> 
> > @@ -7307,6 +7307,7 @@ get_template_parm_object (tree expr, tsubst_flags_t
> > complain)
> >         hash_map_safe_put<hm_ggc> (tparm_obj_values, decl, copy);
> >       }
> >   +  set_originating_module (decl);
> >     pushdecl_top_level_and_finish (decl, expr);
> 
> this is wrong.  You're attaching the decl to the current module. which will
> mean conflicts when reading in such VAR_DECLs for the same NTTP from different
> modules.  Your test case might be hiding that as you have an interface and
> implementation unit from the same module (but you should be getting some kind
> of multiple definition error anyway?)

Makes sense.  Indeed this approach falls apart for the following testcase
which uses the same NTTP argument in two different modules, for the
reasons you mentioned:

$ cat 100616_a.H
template<auto> struct C { };
struct A { };

$ cat 100616_b.C
export module pr100616_b;
import "100616_a.H";
export C<A{}> c1;

$ cat 100616_c.C
export module pr100616_c;
import "100616_a.H";
export C<A{}> c2;

$ cat 100616_d.C
import pr100616_b;
import pr100616_c;
using type = decltype(c1);
using type = decltype(c2); // bogus error: types of c1 and c2 don't match

> 
> 
> >       return decl;
> > @@ -29150,9 +29151,10 @@ finish_concept_definition (cp_expr id, tree init)
> >   static tree
> >   listify (tree arg)
> >   {
> > -  tree std_init_list = get_namespace_binding (std_node,
> > init_list_identifier);
> > +  tree std_init_list = lookup_qualified_name (std_node,
> > init_list_identifier);
> >   -  if (!std_init_list || !DECL_CLASS_TEMPLATE_P (std_init_list))
> > +  if (std_init_list == error_mark_node
> > +      || !DECL_CLASS_TEMPLATE_P (std_init_list))
> >       {
> >         gcc_rich_location richloc (input_location);
> >         maybe_add_include_fixit (&richloc, "<initializer_list>", false);

What do you think about this independent change to use
lookup_qualified_name instead of get_namespace_binding in listify so
that the lookup for std::initializer_list is import-aware, which seems
to fix PR102576?

> > diff --git a/gcc/testsuite/g++.dg/modules/pr100616_a.C
> > b/gcc/testsuite/g++.dg/modules/pr100616_a.C
> > new file mode 100644
> > index 00000000000..788af2eb533
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/pr100616_a.C
> > @@ -0,0 +1,8 @@
> > +// PR c++/100616
> > +// { dg-additional-options "-std=c++20 -fmodules-ts" }
> > +// { dg-module-cmi pr100616 }
> > +export module pr100616;
> > +
> > +template<auto> struct C { };
> > +struct A { };
> > +C<A{}> c1;
> > diff --git a/gcc/testsuite/g++.dg/modules/pr100616_b.C
> > b/gcc/testsuite/g++.dg/modules/pr100616_b.C
> > new file mode 100644
> > index 00000000000..fc89cd08ac5
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/pr100616_b.C
> > @@ -0,0 +1,8 @@
> > +// PR c++/100616
> > +// { dg-additional-options "-std=c++20 -fmodules-ts" }
> > +module pr100616;
> > +
> > +C<A{}> c2;
> > +
> > +using type = decltype(c1);
> > +using type = decltype(c2);
> > diff --git a/gcc/testsuite/g++.dg/modules/pr102576_a.H
> > b/gcc/testsuite/g++.dg/modules/pr102576_a.H
> > new file mode 100644
> > index 00000000000..87ba9b52031
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/pr102576_a.H
> > @@ -0,0 +1,5 @@
> > +// PR c++/102576
> > +// { dg-additional-options -fmodule-header }
> > +// { dg-module-cmi {} }
> > +
> > +#include <initializer_list>
> > diff --git a/gcc/testsuite/g++.dg/modules/pr102576_b.C
> > b/gcc/testsuite/g++.dg/modules/pr102576_b.C
> > new file mode 100644
> > index 00000000000..10251ed5304
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/pr102576_b.C
> > @@ -0,0 +1,9 @@
> > +// PR c++/102576
> > +// { dg-additional-options -fmodules-ts }
> > +
> > +import "pr102576_a.H";
> > +
> > +int main() {
> > +  for (int i : {1, 2, 3})
> > +    ;
> > +}
> 
> -- 
> Nathan Sidwell
> 
> 

  reply	other threads:[~2022-09-28 14:42 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-22 18:25 Patrick Palka
2022-09-22 19:13 ` Nathan Sidwell
2022-09-23 13:32   ` Patrick Palka
2022-09-26 14:08     ` Nathan Sidwell
2022-09-26 14:46       ` Nathan Sidwell
2022-09-26 18:26         ` Patrick Palka
2022-09-26 19:05           ` Patrick Palka
2022-09-27 11:49             ` Nathan Sidwell
2022-09-28 14:42               ` Patrick Palka [this message]
2022-09-28 20:51                 ` Nathan Sidwell

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=5943173d-4a56-c759-d1c2-80f389723ff1@idea \
    --to=ppalka@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=jason@redhat.com \
    --cc=nathan@acm.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).