From: Nathaniel Shead <nathanieloshead@gmail.com>
To: Jason Merrill <jason@redhat.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH v6] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286]
Date: Fri, 13 Oct 2023 09:05:34 +1100 [thread overview]
Message-ID: <65286db4.630a0220.196a2.79cb@mx.google.com> (raw)
In-Reply-To: <f8331fcc-ed70-479f-9b3f-fb3a86657899@redhat.com>
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();
next prev parent reply other threads:[~2023-10-12 22:05 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-29 13:35 [PATCH] c++: Check for indirect change of active union member in constexpr [PR101631] Nathaniel Shead
2023-08-30 20:28 ` Jason Merrill
2023-09-01 12:22 ` [PATCH v2] c++: Catch " Nathaniel Shead
2023-09-17 12:46 ` Nathaniel Shead
2023-09-19 21:25 ` Jason Merrill
2023-09-20 0:55 ` Nathaniel Shead
2023-09-20 19:23 ` Jason Merrill
2023-09-21 13:41 ` [PATCH v3] " Nathaniel Shead
2023-09-22 13:21 ` Jason Merrill
2023-09-22 15:01 ` [PATCH v4] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286] Nathaniel Shead
2023-09-23 0:38 ` Nathaniel Shead
2023-09-23 6:40 ` Jonathan Wakely
2023-09-23 7:30 ` [PATCH] libstdc++: Ensure active union member is correctly set Nathaniel Shead
2023-09-23 10:52 ` Jonathan Wakely
2023-09-27 14:13 ` Jonathan Wakely
2023-09-28 23:25 ` Nathaniel Shead
2023-09-29 9:32 ` Jonathan Wakely
2023-09-29 15:06 ` Jonathan Wakely
2023-09-29 16:29 ` Nathaniel Shead
2023-09-29 16:46 ` Jonathan Wakely
2023-10-21 14:45 ` Jonathan Wakely
2023-10-09 1:03 ` [PATCH v4] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286] Nathaniel Shead
2023-10-09 20:46 ` Jason Merrill
2023-10-10 13:48 ` [PATCH v5] " Nathaniel Shead
2023-10-12 8:53 ` [PATCH v6] " Nathaniel Shead
2023-10-12 20:24 ` Jason Merrill
2023-10-12 22:05 ` Nathaniel Shead [this message]
2023-10-20 3:23 ` Jason Merrill
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=65286db4.630a0220.196a2.79cb@mx.google.com \
--to=nathanieloshead@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jason@redhat.com \
/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).