From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x82e.google.com (mail-qt1-x82e.google.com [IPv6:2607:f8b0:4864:20::82e]) by sourceware.org (Postfix) with ESMTPS id F0D053858C2D for ; Tue, 27 Sep 2022 11:49:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org F0D053858C2D Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=acm.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-qt1-x82e.google.com with SMTP id r20so5781471qtn.12 for ; Tue, 27 Sep 2022 04:49:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :sender:from:to:cc:subject:date; bh=SV0Lkh11hGu1eQZu7CRA61i3g0rARekAUk1XeAgO2dg=; b=BITNhVeQuozd3Z1WuMWUC8AFnzoWQ9oWygLuWnk7qy4S2mVOmaj4ogB+PIJAfz4sM7 KVfQfjvxnXM9HTU38ovsvMZqFVqH2jy3Q+P8doyKoaYu4SMBRmf0j9Z8GTG0WISKhElQ UtIPFWGrH5NpXRCEF7Qc3QtJ8kRoDNW1iDeYojeIuGGHM0fn82csW2st8l0dbFkHzsqn /MZ2gpoQ4lj0XjqTVraeBB89yu1g+hCuZ49u+YYrzW+4GnQfmTzCGs6LVvkfr1yDoRcE 4ocbREJsQHTp8c0nXwW68QFH90d52hqufSMNxUHMVW+w81oUxRMqEncA5YuSILimu/Ee 7tmg== 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:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :sender:x-gm-message-state:from:to:cc:subject:date; bh=SV0Lkh11hGu1eQZu7CRA61i3g0rARekAUk1XeAgO2dg=; b=x+AB7C1YFKPhFC/KXFu2Rx6B8MpTIaBaOPNx0uoaR6JIO4rMBPZuwS3wDPNG8TYSWI 8TFy5o0HD4UhkITgxtgswBRkysM5F6cyAAib4v0JeM45e5htHnkH71W2dncJHvaEJa0o KrxbeVMmkUAvg8A/iKpS5jAccGkCtzbtTj5VXXQnV50TvM4uCs0G2sh12ScJwzimFdAZ wBfSoLgdu3GIWDP2zxAUwVRngelj8cTBCiSj/WSht4LkKt4Kr/Bw1QilUktXVNobXFFu p7qbzE2Kee9yirxSDP5eP0RU/55Vq9G7aCy31lv6XNEPZKkvFnYH6cp8TbtM+hG495vC 1bSA== X-Gm-Message-State: ACrzQf36etphhArPlutGYhkeoXcW6bQqni6L0bMkEWxQi3c6dCL1lvR3 tXHFpO3LsSw259k1OiosYOMRPhbyUnE= X-Google-Smtp-Source: AMsMyM6WfAYTC7bvEW9vyi2qAzeww/2LwrtQLBI5zH9jfXydM+BkVU4APoQ967wi/mcyTvY57yj99w== X-Received: by 2002:a05:622a:138b:b0:35b:b619:b87d with SMTP id o11-20020a05622a138b00b0035bb619b87dmr22016778qtk.146.1664279363913; Tue, 27 Sep 2022 04:49:23 -0700 (PDT) Received: from ?IPV6:2620:10d:c0a3:1407:c96f:a2a3:1408:d018? ([2620:10d:c091:500::6:38f1]) by smtp.googlemail.com with ESMTPSA id d22-20020a05620a205600b006b9c6d590fasm789957qka.61.2022.09.27.04.49.23 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 27 Sep 2022 04:49:23 -0700 (PDT) Sender: Nathan Sidwell Message-ID: Date: Tue, 27 Sep 2022 07:49:22 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.0 Subject: Re: [PATCH] c++ modules: ICE with class NTTP argument [PR100616] To: Patrick Palka Cc: gcc-patches@gcc.gnu.org, jason@redhat.com References: <20220922182502.3218391-1-ppalka@redhat.com> <91a5a188-c4f2-39ff-fff4-8a667f515a19@acm.org> <0745ddf7-8e48-909b-d8e2-a9a1e4064cef@idea> <8728ddf0-ba58-60b3-9083-781901e350f9@idea> <9c72067f-c3ff-50d7-cd07-ca1eec819f82@idea> Content-Language: en-US From: Nathan Sidwell In-Reply-To: <9c72067f-c3ff-50d7-cd07-ca1eec819f82@idea> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3040.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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 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 struct C { }; >>>> struct A { }; >>>> C c1; // #1 >>>> C 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 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. > @@ -7307,6 +7307,7 @@ get_template_parm_object (tree expr, tsubst_flags_t complain) > hash_map_safe_put (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?) > > 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, "", false); > 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 struct C { }; > +struct A { }; > +C 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 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 > 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