Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
c++: Check for indirect change of active union member in constexpr [P…
…R101631,PR102286] 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. -- >8 -- This patch adds checks for attempting to change the active member of a union by methods other than a member access expression. To be able to properly distinguish `*(&u.a) = ` from `u.a = `, this patch redoes the solution for c++/59950 to avoid extranneous *&; it seems that the only case that needed the workaround was when copying empty classes. This patch also ensures that constructors for a union field mark that field as the active member before entering the call itself; this ensures that modifications of the field within the constructor's body don't cause false positives (as these will not appear to be member access expressions). This means that we no longer need to start the lifetime of empty union members after the constructor body completes. As a drive-by fix, this patch also ensures that value-initialised unions are considered to have activated their initial member for the purpose of checking stores and accesses, which catches some additional mistakes pre-C++20. PR c++/101631 PR c++/102286 gcc/cp/ChangeLog: * call.cc (build_over_call): Fold more indirect refs for trivial assignment op. * class.cc (type_has_non_deleted_trivial_default_ctor): Create. * constexpr.cc (cxx_eval_call_expression): Start lifetime of union member before entering constructor. (cxx_eval_component_reference): Check against first member of value-initialised union. (cxx_eval_store_expression): Activate member for value-initialised union. Check for accessing inactive union member indirectly. * cp-tree.h (type_has_non_deleted_trivial_default_ctor): Forward declare. gcc/testsuite/ChangeLog: * g++.dg/cpp1y/constexpr-89336-3.C: Fix union initialisation. * g++.dg/cpp1y/constexpr-union6.C: New test. * g++.dg/cpp1y/constexpr-union7.C: New test. * g++.dg/cpp2a/constexpr-union2.C: New test. * g++.dg/cpp2a/constexpr-union3.C: New test. * g++.dg/cpp2a/constexpr-union4.C: New test. * g++.dg/cpp2a/constexpr-union5.C: New test. * g++.dg/cpp2a/constexpr-union6.C: New test. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
- Loading branch information
1 parent
c2d62cd
commit 6d98efc