Re: [PATCH v6] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286]
On 10/12/23 18:05, Nathaniel Shead wrote:
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(, S{ 1, 2 });
return u.s.b;
}
static_assert(test1() == 2);
The initialisation of 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 =
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
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(); // { 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
Re: [PATCH v6] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286]
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(, S{ 1, 2 });
> > > return u.s.b;
> > >}
> > >static_assert(test1() == 2);
> > >
> > > The initialisation of 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 =
> > > 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
> > > >
> > > > 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(); // { 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).
>
Re: [PATCH v6] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286]
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(, S{ 1, 2 });
return u.s.b;
}
static_assert(test1() == 2);
The initialisation of 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 =
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
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(); // { 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
