From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x535.google.com (mail-pg1-x535.google.com [IPv6:2607:f8b0:4864:20::535]) by sourceware.org (Postfix) with ESMTPS id 85DE8385696E for ; Thu, 12 Oct 2023 22:05:42 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 85DE8385696E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-pg1-x535.google.com with SMTP id 41be03b00d2f7-58d26cfe863so1069811a12.2 for ; Thu, 12 Oct 2023 15:05:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1697148341; x=1697753141; darn=gcc.gnu.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:subject:cc:to:from:date:message-id:from:to :cc:subject:date:message-id:reply-to; bh=XYZfOtD3cBqmwm8WB3ouO+64ipp3uUMZb5a0p1fjpYo=; b=HiGi0qtwSrxTAXvcb23N+iQeIGPrbFWMM4A5IMjb44ctJPDd2ZnoekiVy+dAw1QerK 09tcMh2bPa1hDVy1ae+jLHWeDoQnfjk/+SGVBAIhbiX3rnsja8ee1zp7nMnfpD6P2PM4 WHHMmmsdAUFiUgKU31CWy9MrBSWE04O+hm3XdZcxTLnT398n8Qt7/SkIyk7ubJXNc28I 8+WHk1kJ9wNOCgmqwEAVRU/th8CjxD3jT4jEVRMR+TZBBkYVsXBbY3UzUjcEMk87tK2t +5DXKOR1AGFVIraDcT8lGptVmY1bvFfVrHx49koKau9PlDTKG2NTtrkI8KcRh0jpEX7j Khqg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697148341; x=1697753141; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:subject:cc:to:from:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=XYZfOtD3cBqmwm8WB3ouO+64ipp3uUMZb5a0p1fjpYo=; b=Zw1u2TGXsRlgDtPF9LRuqCyUJcX3zNfu1LsKcnDjRx2++KHJvj4RJ7m8+cHKex8MQ+ YE1tQhWVxhmdBQwe12A8u+bnDFHY9pR4m5DbpPiHARkrNAKWBlLOlITdxxoBNNMo5fiM gCqqSKD/T08M5Gq3u5KPxyBQwxX03Ft+d2sMwMxo+HcSRsO0hQChyO4tOQYFAMJfM99m Yt3e3hBqaOgzVFL+Cx4uQfky4zMt60tyZunlguBY1Iw2S7fLc+4Rj2wL3fUi2bIj45hL GHlmWHTpXVjufQ73jKo/+xUMebPrWWTcKPY7hxYRKph7QhmLu4LEo+7J9X9pKciaejCn +vYg== X-Gm-Message-State: AOJu0YzsRnP9Im9qdUo43lQyfSmdhhp4eCVKdypkFbIbcySw3ShgP9VD T4zeUxkTe1drCbc+4f5Ej/I= X-Google-Smtp-Source: AGHT+IFaI1RO8uztmdHRCYgjabXbiiYRAchbIuDzNvt98rIMkKC2bhgO3txs8VOU524J5WhmN7E+xg== X-Received: by 2002:a05:6a20:7291:b0:157:b453:dbb9 with SMTP id o17-20020a056a20729100b00157b453dbb9mr27391470pzk.6.1697148341229; Thu, 12 Oct 2023 15:05:41 -0700 (PDT) Received: from Thaum. (124-150-88-161.tpgi.com.au. [124.150.88.161]) by smtp.gmail.com with ESMTPSA id a29-20020a63705d000000b0058958ea2aaesm2191421pgn.83.2023.10.12.15.05.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 12 Oct 2023 15:05:40 -0700 (PDT) Message-ID: <65286db4.630a0220.196a2.79cb@mx.google.com> X-Google-Original-Message-ID: Date: Fri, 13 Oct 2023 09:05:34 +1100 From: Nathaniel Shead To: Jason Merrill Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH v6] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286] References: <65235160.170a0220.4cbca.5c7d@mx.google.com> <65255623.620a0220.39bb2.41d9@mx.google.com> <6527b428.170a0220.810bd.457e@mx.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,KAM_SHORT,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 Thu, Oct 12, 2023 at 04:24:00PM -0400, Jason Merrill wrote: > On 10/12/23 04:53, Nathaniel Shead wrote: > > On Wed, Oct 11, 2023 at 12:48:12AM +1100, Nathaniel Shead wrote: > > > On Mon, Oct 09, 2023 at 04:46:46PM -0400, Jason Merrill wrote: > > > > On 10/8/23 21:03, Nathaniel Shead wrote: > > > > > Ping for https://gcc.gnu.org/pipermail/gcc-patches/2023-September/631203.html > > > > > > > > > > + && (TREE_CODE (t) == MODIFY_EXPR > > > > > + /* Also check if initializations have implicit change of active > > > > > + member earlier up the access chain. */ > > > > > + || !refs->is_empty()) > > > > > > > > I'm not sure what the cumulative point of these two tests is. TREE_CODE (t) > > > > will be either MODIFY_EXPR or INIT_EXPR, and either should be OK. > > > > > > > > As I understand it, the problematic case is something like > > > > constexpr-union2.C, where we're also looking at a MODIFY_EXPR. So what is > > > > this check doing? > > > > > > The reasoning was to correctly handle cases like the the following (in > > > constexpr-union6.C): > > > > > > constexpr int test1() { > > > U u {}; > > > std::construct_at(&u.s, S{ 1, 2 }); > > > return u.s.b; > > > } > > > static_assert(test1() == 2); > > > > > > The initialisation of &u.s here is not a member access expression within > > > the call to std::construct_at, since it's just a pointer, but this code > > > is still legal; in general, an INIT_EXPR to initialise a union member > > > should always be OK (I believe?), hence constraining to just > > > MODIFY_EXPR. > > > > > > However, just that would then (incorrectly) allow all the following > > > cases in that test to compile, such as > > > > > > constexpr int test2() { > > > U u {}; > > > int* p = &u.s.b; > > > std::construct_at(p, 5); > > > return u.s.b; > > > } > > > constexpr int x2 = test2(); > > > > > > since the INIT_EXPR is really only initialising 'b', but the implicit > > > "modification" of active member to 'u.s' is illegal. > > > > > > Maybe a better way of expressing this condition would be something like > > > this? > > > > > > /* An INIT_EXPR of the last member in an access chain is always OK, > > > but still check implicit change of members earlier on; see > > > cpp2a/constexpr-union6.C. */ > > > && !(TREE_CODE (t) == INIT_EXPR && refs->is_empty ()) > > > > > > Otherwise I'll see if I can rework some of the other conditions instead. > > > > > > > Incidentally, I think constexpr-union6.C could use a test where we pass &u.s > > > > to a function other than construct_at, and then try (and fail) to assign to > > > > the b member from that function. > > > > > > > > Jason > > > > > > > > > > Sounds good; I've added the following test: > > > > > > constexpr void foo(S* s) { > > > s->b = 10; // { dg-error "accessing .U::s. member instead of initialized .U::k." } > > > } > > > constexpr int test3() { > > > U u {}; > > > foo(&u.s); // { dg-message "in .constexpr. expansion" } > > > return u.s.b; > > > } > > > constexpr int x3 = test3(); // { dg-message "in .constexpr. expansion" } > > > > > > Incidentally I found this particular example caused a very unhelpful > > > error + ICE due to reporting that S could not be value-initialized in > > > the current version of the patch. The updated version below fixes that > > > by using 'build_zero_init' instead -- is this an appropriate choice > > > here? > > > > > > A similar (but unrelated) issue is with e.g. > > > struct S { const int a; int b; }; > > > union U { int k; S s; }; > > > > > > constexpr int test() { > > > U u {}; > > > return u.s.b; > > > } > > > constexpr int x = test(); > > > > > > giving me this pretty unhelpful error message: > > > > > > /home/ns/main.cpp:8:23: in ‘constexpr’ expansion of ‘test()’ > > > /home/ns/main.cpp:6:12: error: use of deleted function ‘S::S()’ > > > 6 | return u.s.b; > > > | ~~^ > > > /home/ns/main.cpp:1:8: note: ‘S::S()’ is implicitly deleted because the default definition would be ill-formed: > > > 1 | struct S { const int a; int b; }; > > > | ^ > > > /home/ns/main.cpp:1:8: error: uninitialised const member in ‘struct S’ > > > /home/ns/main.cpp:1:22: note: ‘const int S::a’ should be initialised > > > 1 | struct S { const int a; int b; }; > > > | ^ > > > /home/ns/main.cpp:8:23: in ‘constexpr’ expansion of ‘test()’ > > > /home/ns/main.cpp:6:12: error: use of deleted function ‘S::S()’ > > > 6 | return u.s.b; > > > | ~~^ > > > /home/ns/main.cpp:8:23: in ‘constexpr’ expansion of ‘test()’ > > > /home/ns/main.cpp:6:12: error: use of deleted function ‘S::S()’ > > > > > > but I'll try and fix this separately (it exists on current trunk without > > > this patch as well). > > > > While attempting to fix this I found a much better way of handling > > value-initialised unions. Here's a new version of the patch which also > > includes the fix for accessing the wrong member of a value-initialised > > union as well. > > > > Additionally includes an `auto_diagnostic_group` for the `inform` > > diagnostics as Marek helpfully informed me about in my other patch. > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > > @@ -4496,21 +4491,36 @@ cxx_eval_component_reference (const constexpr_ctx *ctx, tree t, > > break; > > } > > } > > - if (TREE_CODE (TREE_TYPE (whole)) == UNION_TYPE > > - && CONSTRUCTOR_NELTS (whole) > 0) > > + if (TREE_CODE (TREE_TYPE (whole)) == UNION_TYPE) > > { > > - /* DR 1188 says we don't have to deal with this. */ > > - if (!ctx->quiet) > > + if (CONSTRUCTOR_NELTS (whole) > 0) > > { > > - constructor_elt *cep = CONSTRUCTOR_ELT (whole, 0); > > - if (cep->value == NULL_TREE) > > - error ("accessing uninitialized member %qD", part); > > - else > > - error ("accessing %qD member instead of initialized %qD member in " > > - "constant expression", part, cep->index); > > + /* DR 1188 says we don't have to deal with this. */ > > + if (!ctx->quiet) > > + { > > + constructor_elt *cep = CONSTRUCTOR_ELT (whole, 0); > > + if (cep->value == NULL_TREE) > > + error ("accessing uninitialized member %qD", part); > > + else > > + error ("accessing %qD member instead of initialized %qD member in " > > + "constant expression", part, cep->index); > > + } > > + *non_constant_p = true; > > + return t; > > + } > > + else if (!CONSTRUCTOR_NO_CLEARING (whole)) > > + { > > + /* Value-initialized union, check if looking at the first member. */ > > + tree first = next_aggregate_field (TYPE_FIELDS (TREE_TYPE (whole))); > > + if (pmf ? DECL_NAME (first) != DECL_NAME (part) : first != part) > > You don't need to consider pmf here, since a PMF isn't UNION_TYPE. > Ah right, thanks. > > @@ -6267,23 +6288,74 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > > break; > > } > > + /* Process value-initialization of a union. */ > > + if (code == UNION_TYPE > > + && !CONSTRUCTOR_NO_CLEARING (*valp) > > + && CONSTRUCTOR_NELTS (*valp) == 0) > > + if (tree first = next_aggregate_field (TYPE_FIELDS (type))) > > + CONSTRUCTOR_APPEND_ELT (CONSTRUCTOR_ELTS (*valp), first, NULL_TREE); > > Isn't this adding an uninitialized member instead of value-initialized? > > Jason > This is equivalent to what was going to happen at the end of the loop when 'get_or_insert_ctor_field (*valp, index)' is called regardless. A value-initialized subobject is then created at the start of the loop (based on 'no_zero_init') replacing the NULL_TREE here. I can try expanding the comment to clarify this perhaps. Here are two tests that validate this is working as intended as well: union U1 { int a; int b; }; union U2 { U1 u; }; union U3 { U2 u; }; constexpr int foo() { U3 u {}; int* p = &u.u.u.a; *p = 10; return *p; } constexpr int x = foo(); constexpr int bar() { U3 u {}; int* p = &u.u.u.b; *p = 10; // { dg-error "accessing" } return *p; } constexpr int y = bar();