* [PATCH v4] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286] [not found] ` <ZQ2sQNoVz0rblzDN@Thaum.localdomain> @ 2023-09-23 0:38 ` Nathaniel Shead 2023-09-23 6:40 ` Jonathan Wakely 0 siblings, 1 reply; 11+ messages in thread From: Nathaniel Shead @ 2023-09-23 0:38 UTC (permalink / raw) To: Jason Merrill, libstdc++; +Cc: gcc-patches Now that bootstrap has finished, I have gotten regressions in the following libstdc++ tests: Running libstdc++:libstdc++-dg/conformance.exp ... FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++23 (test for excess errors) FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++26 (test for excess errors) FAIL: 20_util/variant/constexpr.cc -std=gnu++20 (test for excess errors) FAIL: 20_util/variant/constexpr.cc -std=gnu++26 (test for excess errors) FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++20 (test for excess errors) FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++26 (test for excess errors) FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++20 (test for excess errors) FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++26 (test for excess errors) FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc -std=gnu++20 (test for excess errors) FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc -std=gnu++26 (test for excess errors) FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++20 (test for excess errors) FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++26 (test for excess errors) FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++23 (test for excess errors) UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++23 compilation failed to produce executable FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++26 (test for excess errors) UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++26 compilation failed to produce executable On investigation though it looks like the issue might be with libstdc++ rather than the patch itself; running the failing tests using clang with libstdc++ also produces similar errors, and my reading of the code suggests that this is correct. What's the way forward here? Should I look at creating a patch to fix the libstdc++ issues before resubmitting this patch for the C++ frontend? Or should I submit a version of this patch without the `std::construct_at` changes and wait till libstdc++ gets fixed for that? On Sat, Sep 23, 2023 at 01:01:20AM +1000, Nathaniel Shead wrote: > On Fri, Sep 22, 2023 at 02:21:15PM +0100, Jason Merrill wrote: > > On 9/21/23 09:41, Nathaniel Shead wrote: > > > I've updated the error messages, and also fixed another bug I found > > > while retesting (value-initialised unions weren't considered to have any > > > active member yet). > > > > > > 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, which catches some additional mistakes pre-C++20. > > > > > > PR c++/101631 > > > > > > 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_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/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. > > > > > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > > > --- > > > gcc/cp/call.cc | 11 +- > > > gcc/cp/class.cc | 8 ++ > > > gcc/cp/constexpr.cc | 129 +++++++++++++----- > > > gcc/cp/cp-tree.h | 1 + > > > .../g++.dg/cpp1y/constexpr-89336-3.C | 2 +- > > > gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C | 13 ++ > > > gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C | 30 ++++ > > > gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C | 45 ++++++ > > > gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C | 29 ++++ > > > gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C | 71 ++++++++++ > > > 10 files changed, 296 insertions(+), 43 deletions(-) > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C > > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C > > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C > > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C > > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C > > > > > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc > > > index e8dafbd8ba6..c1fb8807d3f 100644 > > > --- a/gcc/cp/call.cc > > > +++ b/gcc/cp/call.cc > > > @@ -10330,10 +10330,7 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) > > > && DECL_OVERLOADED_OPERATOR_IS (fn, NOP_EXPR) > > > && trivial_fn_p (fn)) > > > { > > > - /* Don't use cp_build_fold_indirect_ref, op= returns an lvalue even if > > > - the object argument isn't one. */ > > > - tree to = cp_build_indirect_ref (input_location, argarray[0], > > > - RO_ARROW, complain); > > > + tree to = cp_build_fold_indirect_ref (argarray[0]); > > > tree type = TREE_TYPE (to); > > > tree as_base = CLASSTYPE_AS_BASE (type); > > > tree arg = argarray[1]; > > > @@ -10341,7 +10338,11 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) > > > if (is_really_empty_class (type, /*ignore_vptr*/true)) > > > { > > > - /* Avoid copying empty classes. */ > > > + /* Avoid copying empty classes, but ensure op= returns an lvalue even > > > + if the object argument isn't one. This isn't needed in other cases > > > + since MODIFY_EXPR is always considered an lvalue. */ > > > + to = cp_build_addr_expr (to, tf_none); > > > + to = cp_build_indirect_ref (input_location, to, RO_ARROW, complain); > > > val = build2 (COMPOUND_EXPR, type, arg, to); > > > suppress_warning (val, OPT_Wunused); > > > } > > > diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc > > > index b71333af1f8..e31aeb8e68b 100644 > > > --- a/gcc/cp/class.cc > > > +++ b/gcc/cp/class.cc > > > @@ -5688,6 +5688,14 @@ type_has_virtual_destructor (tree type) > > > return (dtor && DECL_VIRTUAL_P (dtor)); > > > } > > > +/* True iff class TYPE has a non-deleted trivial default > > > + constructor. */ > > > + > > > +bool type_has_non_deleted_trivial_default_ctor (tree type) > > > +{ > > > + return TYPE_HAS_TRIVIAL_DFLT (type) && locate_ctor (type); > > > +} > > > + > > > /* Returns true iff T, a class, has a move-assignment or > > > move-constructor. Does not lazily declare either. > > > If USER_P is false, any move function will do. If it is true, the > > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > > > index a673a6022f1..5e50fc2c792 100644 > > > --- a/gcc/cp/constexpr.cc > > > +++ b/gcc/cp/constexpr.cc > > > @@ -3146,40 +3146,34 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > > > cxx_set_object_constness (ctx, new_obj, /*readonly_p=*/false, > > > non_constant_p, overflow_p); > > > + /* If this is a constructor, we are beginning the lifetime of the > > > + object we are initializing. */ > > > + if (new_obj > > > + && DECL_CONSTRUCTOR_P (fun) > > > + && TREE_CODE (new_obj) == COMPONENT_REF > > > + && TREE_CODE (TREE_TYPE (TREE_OPERAND (new_obj, 0))) == UNION_TYPE) > > > + { > > > + tree activate = build2 (INIT_EXPR, TREE_TYPE (new_obj), > > > + new_obj, > > > + build_constructor (TREE_TYPE (new_obj), > > > + NULL)); > > > + cxx_eval_constant_expression (ctx, activate, > > > + lval, non_constant_p, overflow_p); > > > + ggc_free (activate); > > > + } > > > + > > > tree jump_target = NULL_TREE; > > > cxx_eval_constant_expression (&call_ctx, body, > > > vc_discard, non_constant_p, overflow_p, > > > &jump_target); > > > if (DECL_CONSTRUCTOR_P (fun)) > > > - { > > > - /* This can be null for a subobject constructor call, in > > > - which case what we care about is the initialization > > > - side-effects rather than the value. We could get at the > > > - value by evaluating *this, but we don't bother; there's > > > - no need to put such a call in the hash table. */ > > > - result = lval ? ctx->object : ctx->ctor; > > > - > > > - /* If we've just evaluated a subobject constructor call for an > > > - empty union member, it might not have produced a side effect > > > - that actually activated the union member. So produce such a > > > - side effect now to ensure the union appears initialized. */ > > > - if (!result && new_obj > > > - && TREE_CODE (new_obj) == COMPONENT_REF > > > - && TREE_CODE (TREE_TYPE > > > - (TREE_OPERAND (new_obj, 0))) == UNION_TYPE > > > - && is_really_empty_class (TREE_TYPE (new_obj), > > > - /*ignore_vptr*/false)) > > > - { > > > - tree activate = build2 (MODIFY_EXPR, TREE_TYPE (new_obj), > > > - new_obj, > > > - build_constructor (TREE_TYPE (new_obj), > > > - NULL)); > > > - cxx_eval_constant_expression (ctx, activate, lval, > > > - non_constant_p, overflow_p); > > > - ggc_free (activate); > > > - } > > > - } > > > + /* This can be null for a subobject constructor call, in > > > + which case what we care about is the initialization > > > + side-effects rather than the value. We could get at the > > > + value by evaluating *this, but we don't bother; there's > > > + no need to put such a call in the hash table. */ > > > + result = lval ? ctx->object : ctx->ctor; > > > else if (VOID_TYPE_P (TREE_TYPE (res))) > > > result = void_node; > > > else > > > @@ -6127,6 +6121,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > > > mutable_p) > > > && const_object_being_modified == NULL_TREE) > > > const_object_being_modified = probe; > > > + > > > + /* Track named member accesses for unions to validate modifications > > > + that change active member. */ > > > + if (!evaluated && TREE_CODE (probe) == COMPONENT_REF) > > > + vec_safe_push (refs, probe); > > > + else > > > + vec_safe_push (refs, NULL_TREE); > > > + > > > vec_safe_push (refs, elt); > > > vec_safe_push (refs, TREE_TYPE (probe)); > > > probe = ob; > > > @@ -6135,6 +6137,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > > > case REALPART_EXPR: > > > gcc_assert (probe == target); > > > + vec_safe_push (refs, NULL_TREE); > > > vec_safe_push (refs, probe); > > > vec_safe_push (refs, TREE_TYPE (probe)); > > > probe = TREE_OPERAND (probe, 0); > > > @@ -6142,6 +6145,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > > > case IMAGPART_EXPR: > > > gcc_assert (probe == target); > > > + vec_safe_push (refs, NULL_TREE); > > > vec_safe_push (refs, probe); > > > vec_safe_push (refs, TREE_TYPE (probe)); > > > probe = TREE_OPERAND (probe, 0); > > > @@ -6230,6 +6234,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > > > enum tree_code code = TREE_CODE (type); > > > tree reftype = refs->pop(); > > > tree index = refs->pop(); > > > + bool is_access_expr = refs->pop() != NULL_TREE; > > > if (code == COMPLEX_TYPE) > > > { > > > @@ -6270,21 +6275,72 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > > > type = reftype; > > > - if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp) > > > - && CONSTRUCTOR_ELT (*valp, 0)->index != index) > > > + if (code == UNION_TYPE > > > + && TREE_CODE (t) == MODIFY_EXPR > > > + && (CONSTRUCTOR_NELTS (*valp) == 0 > > > + || CONSTRUCTOR_ELT (*valp, 0)->index != index)) > > > { > > > - if (cxx_dialect < cxx20) > > > + /* Probably a change of active member for a union. Ensure that > > > + this is valid. */ > > > + no_zero_init = true; > > > + > > > + tree init = *valp; > > > + if (!CONSTRUCTOR_NO_CLEARING (*valp) && CONSTRUCTOR_NELTS (*valp) == 0) > > > + /* This is a value-initialized union, process the initialization > > > + to ensure that the active member is properly set. */ > > > + init = build_value_init (TREE_TYPE (*valp), tf_warning_or_error); > > > + > > > + bool has_active_member = CONSTRUCTOR_NELTS (init) != 0; > > > + tree inner = strip_array_types (reftype); > > > + > > > + if (has_active_member && CONSTRUCTOR_ELT (init, 0)->index == index) > > > + /* After handling value-initialization, this wasn't actually a > > > + change of active member like we initially thought. */ > > > + no_zero_init = false; > > > + else if (has_active_member && cxx_dialect < cxx20) > > > { > > > if (!ctx->quiet) > > > error_at (cp_expr_loc_or_input_loc (t), > > > "change of the active member of a union " > > > - "from %qD to %qD", > > > - CONSTRUCTOR_ELT (*valp, 0)->index, > > > + "from %qD to %qD is not a constant expression " > > > + "before C++20", > > > + CONSTRUCTOR_ELT (init, 0)->index, > > > index); > > > *non_constant_p = true; > > > } > > > - else if (TREE_CODE (t) == MODIFY_EXPR > > > - && CONSTRUCTOR_NO_CLEARING (*valp)) > > > + else if (!is_access_expr > > > + || (CLASS_TYPE_P (inner) > > > + && !type_has_non_deleted_trivial_default_ctor (inner))) > > > + { > > > + /* Diagnose changing active union member after initialization > > > + without a valid member access expression, as described in > > > + [class.union.general] p5. */ > > > + if (!ctx->quiet) > > > + { > > > + if (has_active_member) > > > + error_at (cp_expr_loc_or_input_loc (t), > > > + "accessing %qD member instead of initialized " > > > + "%qD member in constant expression", > > > + index, CONSTRUCTOR_ELT (init, 0)->index); > > > + else > > > + error_at (cp_expr_loc_or_input_loc (t), > > > + "accessing uninitialized member %qD", > > > + index); > > > + if (is_access_expr) > > > + inform (DECL_SOURCE_LOCATION (index), > > > + "%qD does not implicitly begin its lifetime " > > > + "because %qT does not have a non-deleted " > > > + "trivial default constructor", > > > + index, inner); > > > > It now occurs to me that this should suggest using placement new instead. > > And that we should test using placement new to begin the lifetime of such > > members. > > > > Jason > > > > Good point. On testing placement new I found I wasn't checking for > indirect change of active member during initialisation either, but this > was pretty trivial to add. How does this look? > > Bootstrap + regtest ongoing, but dg.exp completed with no errors. I'm > away from my computer next week so I thought I'd send this through now > anyway. > > -- >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, 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_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/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> > --- > gcc/cp/call.cc | 11 +- > gcc/cp/class.cc | 8 ++ > gcc/cp/constexpr.cc | 135 +++++++++++++----- > gcc/cp/cp-tree.h | 1 + > .../g++.dg/cpp1y/constexpr-89336-3.C | 2 +- > gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C | 13 ++ > gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C | 30 ++++ > gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C | 45 ++++++ > gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C | 29 ++++ > gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C | 71 +++++++++ > gcc/testsuite/g++.dg/cpp2a/constexpr-union6.C | 43 ++++++ > 11 files changed, 345 insertions(+), 43 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union6.C > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc > index e8dafbd8ba6..c1fb8807d3f 100644 > --- a/gcc/cp/call.cc > +++ b/gcc/cp/call.cc > @@ -10330,10 +10330,7 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) > && DECL_OVERLOADED_OPERATOR_IS (fn, NOP_EXPR) > && trivial_fn_p (fn)) > { > - /* Don't use cp_build_fold_indirect_ref, op= returns an lvalue even if > - the object argument isn't one. */ > - tree to = cp_build_indirect_ref (input_location, argarray[0], > - RO_ARROW, complain); > + tree to = cp_build_fold_indirect_ref (argarray[0]); > tree type = TREE_TYPE (to); > tree as_base = CLASSTYPE_AS_BASE (type); > tree arg = argarray[1]; > @@ -10341,7 +10338,11 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain) > > if (is_really_empty_class (type, /*ignore_vptr*/true)) > { > - /* Avoid copying empty classes. */ > + /* Avoid copying empty classes, but ensure op= returns an lvalue even > + if the object argument isn't one. This isn't needed in other cases > + since MODIFY_EXPR is always considered an lvalue. */ > + to = cp_build_addr_expr (to, tf_none); > + to = cp_build_indirect_ref (input_location, to, RO_ARROW, complain); > val = build2 (COMPOUND_EXPR, type, arg, to); > suppress_warning (val, OPT_Wunused); > } > diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc > index b71333af1f8..e31aeb8e68b 100644 > --- a/gcc/cp/class.cc > +++ b/gcc/cp/class.cc > @@ -5688,6 +5688,14 @@ type_has_virtual_destructor (tree type) > return (dtor && DECL_VIRTUAL_P (dtor)); > } > > +/* True iff class TYPE has a non-deleted trivial default > + constructor. */ > + > +bool type_has_non_deleted_trivial_default_ctor (tree type) > +{ > + return TYPE_HAS_TRIVIAL_DFLT (type) && locate_ctor (type); > +} > + > /* Returns true iff T, a class, has a move-assignment or > move-constructor. Does not lazily declare either. > If USER_P is false, any move function will do. If it is true, the > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > index a673a6022f1..8e6f60d03d2 100644 > --- a/gcc/cp/constexpr.cc > +++ b/gcc/cp/constexpr.cc > @@ -3146,40 +3146,34 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > cxx_set_object_constness (ctx, new_obj, /*readonly_p=*/false, > non_constant_p, overflow_p); > > + /* If this is a constructor, we are beginning the lifetime of the > + object we are initializing. */ > + if (new_obj > + && DECL_CONSTRUCTOR_P (fun) > + && TREE_CODE (new_obj) == COMPONENT_REF > + && TREE_CODE (TREE_TYPE (TREE_OPERAND (new_obj, 0))) == UNION_TYPE) > + { > + tree activate = build2 (INIT_EXPR, TREE_TYPE (new_obj), > + new_obj, > + build_constructor (TREE_TYPE (new_obj), > + NULL)); > + cxx_eval_constant_expression (ctx, activate, > + lval, non_constant_p, overflow_p); > + ggc_free (activate); > + } > + > tree jump_target = NULL_TREE; > cxx_eval_constant_expression (&call_ctx, body, > vc_discard, non_constant_p, overflow_p, > &jump_target); > > if (DECL_CONSTRUCTOR_P (fun)) > - { > - /* This can be null for a subobject constructor call, in > - which case what we care about is the initialization > - side-effects rather than the value. We could get at the > - value by evaluating *this, but we don't bother; there's > - no need to put such a call in the hash table. */ > - result = lval ? ctx->object : ctx->ctor; > - > - /* If we've just evaluated a subobject constructor call for an > - empty union member, it might not have produced a side effect > - that actually activated the union member. So produce such a > - side effect now to ensure the union appears initialized. */ > - if (!result && new_obj > - && TREE_CODE (new_obj) == COMPONENT_REF > - && TREE_CODE (TREE_TYPE > - (TREE_OPERAND (new_obj, 0))) == UNION_TYPE > - && is_really_empty_class (TREE_TYPE (new_obj), > - /*ignore_vptr*/false)) > - { > - tree activate = build2 (MODIFY_EXPR, TREE_TYPE (new_obj), > - new_obj, > - build_constructor (TREE_TYPE (new_obj), > - NULL)); > - cxx_eval_constant_expression (ctx, activate, lval, > - non_constant_p, overflow_p); > - ggc_free (activate); > - } > - } > + /* This can be null for a subobject constructor call, in > + which case what we care about is the initialization > + side-effects rather than the value. We could get at the > + value by evaluating *this, but we don't bother; there's > + no need to put such a call in the hash table. */ > + result = lval ? ctx->object : ctx->ctor; > else if (VOID_TYPE_P (TREE_TYPE (res))) > result = void_node; > else > @@ -6127,6 +6121,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > mutable_p) > && const_object_being_modified == NULL_TREE) > const_object_being_modified = probe; > + > + /* Track named member accesses for unions to validate modifications > + that change active member. */ > + if (!evaluated && TREE_CODE (probe) == COMPONENT_REF) > + vec_safe_push (refs, probe); > + else > + vec_safe_push (refs, NULL_TREE); > + > vec_safe_push (refs, elt); > vec_safe_push (refs, TREE_TYPE (probe)); > probe = ob; > @@ -6135,6 +6137,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > > case REALPART_EXPR: > gcc_assert (probe == target); > + vec_safe_push (refs, NULL_TREE); > vec_safe_push (refs, probe); > vec_safe_push (refs, TREE_TYPE (probe)); > probe = TREE_OPERAND (probe, 0); > @@ -6142,6 +6145,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > > case IMAGPART_EXPR: > gcc_assert (probe == target); > + vec_safe_push (refs, NULL_TREE); > vec_safe_push (refs, probe); > vec_safe_push (refs, TREE_TYPE (probe)); > probe = TREE_OPERAND (probe, 0); > @@ -6230,6 +6234,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > enum tree_code code = TREE_CODE (type); > tree reftype = refs->pop(); > tree index = refs->pop(); > + bool is_access_expr = refs->pop() != NULL_TREE; > > if (code == COMPLEX_TYPE) > { > @@ -6270,21 +6275,78 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > > type = reftype; > > - if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp) > - && CONSTRUCTOR_ELT (*valp, 0)->index != index) > + /* Check for implicit change of active member for a union. */ > + if (code == UNION_TYPE > + && (TREE_CODE (t) == MODIFY_EXPR > + /* Also check if initializations have implicit change of active > + member earlier up the access chain. */ > + || !refs->is_empty()) > + && (CONSTRUCTOR_NELTS (*valp) == 0 > + || CONSTRUCTOR_ELT (*valp, 0)->index != index)) > { > - if (cxx_dialect < cxx20) > + no_zero_init = true; > + > + tree init = *valp; > + if (TREE_CODE (t) == MODIFY_EXPR > + && !CONSTRUCTOR_NO_CLEARING (*valp) > + && CONSTRUCTOR_NELTS (*valp) == 0) > + /* Modifying a value-initialized union, process the initialization > + to ensure that the active member is properly set. */ > + init = build_value_init (TREE_TYPE (*valp), tf_warning_or_error); > + > + bool has_active_member = CONSTRUCTOR_NELTS (init) != 0; > + tree inner = strip_array_types (reftype); > + > + if (has_active_member && CONSTRUCTOR_ELT (init, 0)->index == index) > + /* After handling value-initialization, this wasn't actually a > + change of active member like we initially thought. */ > + no_zero_init = false; > + else if (has_active_member && cxx_dialect < cxx20) > { > if (!ctx->quiet) > error_at (cp_expr_loc_or_input_loc (t), > "change of the active member of a union " > - "from %qD to %qD", > - CONSTRUCTOR_ELT (*valp, 0)->index, > + "from %qD to %qD is not a constant expression " > + "before C++20", > + CONSTRUCTOR_ELT (init, 0)->index, > index); > *non_constant_p = true; > } > - else if (TREE_CODE (t) == MODIFY_EXPR > - && CONSTRUCTOR_NO_CLEARING (*valp)) > + else if (!is_access_expr > + || (TREE_CODE (t) == MODIFY_EXPR > + && CLASS_TYPE_P (inner) > + && !type_has_non_deleted_trivial_default_ctor (inner))) > + { > + /* Diagnose changing active union member after initialization > + without a valid member access expression, as described in > + [class.union.general] p5. */ > + if (!ctx->quiet) > + { > + if (has_active_member) > + error_at (cp_expr_loc_or_input_loc (t), > + "accessing %qD member instead of initialized " > + "%qD member in constant expression", > + index, CONSTRUCTOR_ELT (init, 0)->index); > + else > + error_at (cp_expr_loc_or_input_loc (t), > + "accessing uninitialized member %qD", > + index); > + if (is_access_expr) > + inform (DECL_SOURCE_LOCATION (index), > + "%qD does not implicitly begin its lifetime " > + "because %qT does not have a non-deleted " > + "trivial default constructor, use " > + "%<std::construct_at%> instead", > + index, inner); > + else > + inform (DECL_SOURCE_LOCATION (index), > + "initializing %qD requires a member access " > + "expression as the left operand of the assignment", > + index); > + } > + *non_constant_p = true; > + } > + else if (has_active_member && CONSTRUCTOR_NO_CLEARING (init)) > { > /* Diagnose changing the active union member while the union > is in the process of being initialized. */ > @@ -6292,11 +6354,10 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t, > error_at (cp_expr_loc_or_input_loc (t), > "change of the active member of a union " > "from %qD to %qD during initialization", > - CONSTRUCTOR_ELT (*valp, 0)->index, > + CONSTRUCTOR_ELT (init, 0)->index, > index); > *non_constant_p = true; > } > - no_zero_init = true; > } > > ctors.safe_push (valp); > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index fd6bf9fd4a0..40aa9f0be2b 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -6815,6 +6815,7 @@ extern bool trivial_default_constructor_is_constexpr (tree); > extern bool type_has_constexpr_default_constructor (tree); > extern bool type_has_constexpr_destructor (tree); > extern bool type_has_virtual_destructor (tree); > +extern bool type_has_non_deleted_trivial_default_ctor (tree); > extern bool classtype_has_move_assign_or_move_ctor_p (tree, bool user_declared); > extern bool classtype_has_non_deleted_move_ctor (tree); > extern tree classtype_has_depr_implicit_copy (tree); > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C > index 9d370ddefcf..6a60966f6f8 100644 > --- a/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C > @@ -18,7 +18,7 @@ constexpr int > bar () > { > union U { int a[5]; long b; }; > - union V { union U u; short v; }; > + union V { short v; union U u; }; > V w {}; > w.v = 5; > w.u.a[3] = w.u.a[1] = w.v; // { dg-error "change of the active member of a union from" "" { target c++17_down } } > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C b/gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C > new file mode 100644 > index 00000000000..ff7ebf19f89 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C > @@ -0,0 +1,13 @@ > +// { dg-do compile { target c++14 } } > + > +union U { > + int a; > + float b; > +}; > + > +constexpr bool foo() { > + U u {}; > + u.b = 1.0f; // { dg-error "change of the active member" "" { target c++17_down } } > + return u.b == 1.0f; > +} > +constexpr bool x = foo(); > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C > new file mode 100644 > index 00000000000..1712395d7e7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C > @@ -0,0 +1,30 @@ > +// PR c++/101631 > +// { dg-do compile { target c++20 } } > + > +struct sso { > + union { > + int buf[10]; > + int* alloc; > + }; > +}; > + > +constexpr bool direct() { > + sso val; > + val.alloc = nullptr; > + val.buf[5] = 42; > + return true; > +} > +constexpr bool ok = direct(); > + > + > +constexpr void perform_assignment(int& left, int right) noexcept { > + left = right; // { dg-error "accessing .+ member instead of initialized" } > +} > + > +constexpr bool indirect() { > + sso val; > + val.alloc = nullptr; > + perform_assignment(val.buf[5], 42); // { dg-message "in .constexpr. expansion" } > + return true; > +} > +constexpr bool err = indirect(); // { dg-message "in .constexpr. expansion" } > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C > new file mode 100644 > index 00000000000..6d30bb2498f > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C > @@ -0,0 +1,45 @@ > +// { dg-do compile { target c++20 } } > + > +struct S > +{ > + union { > + char buf[8]; > + char* ptr; > + }; > + unsigned len; > + > + constexpr S(const char* s, unsigned n) > + { > + char* p; > + if (n > 7) > + p = ptr = new char[n+1]; > + else > + p = buf; > + for (len = 0; len < n; ++len) > + p[len] = s[len]; // { dg-error "accessing uninitialized member" } > + p[len] = '\0'; > + } > + > + constexpr ~S() > + { > + if (len > 7) > + delete[] ptr; > + } > +}; > + > +constexpr bool test1() > +{ > + S s("test", 4); // { dg-message "in .constexpr. expansion" } > + return true; > +} > + > +constexpr bool a = test1(); // { dg-message "in .constexpr. expansion" } > + > + > +constexpr bool test2() > +{ > + S s("hello world", 11); > + return true; > +} > + > +constexpr bool b = test2(); > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C > new file mode 100644 > index 00000000000..429ab203ac2 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C > @@ -0,0 +1,29 @@ > +// { dg-do compile { target c++20 } } > + > +// from [class.union.general] p5 > + > +union A { int x; int y[4]; }; > +struct B { A a; }; > +union C { B b; int k; }; > +constexpr int f() { > + C c; // does not start lifetime of any union member > + c.b.a.y[3] = 4; // OK, S(c.b.a.y[3]) contains c.b and c.b.a.y; > + // creates objects to hold union members c.b and c.b.a.y > + return c.b.a.y[3]; // OK, c.b.a.y refers to newly created object (see [basic.life]) > +} > +constexpr int a = f(); > + > +struct X { const int a; int b; }; > +union Y { X x; int k; };// { dg-message "does not implicitly begin its lifetime" } > +constexpr int g() { > + Y y = { { 1, 2 } }; // OK, y.x is active union member ([class.mem]) > + int n = y.x.a; > + y.k = 4; // OK, ends lifetime of y.x, y.k is active member of union > + > + y.x.b = n; // { dg-error "accessing .* member instead of initialized .* member" } > + // undefined behavior: y.x.b modified outside its lifetime, > + // S(y.x.b) is empty because X's default constructor is deleted, > + // so union member y.x's lifetime does not implicitly start > + return 0; > +} > +constexpr int b = g(); // { dg-message "in .constexpr. expansion" } > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C > new file mode 100644 > index 00000000000..c94d7cc0fc9 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C > @@ -0,0 +1,71 @@ > +// { dg-do compile { target c++20 } } > + > +union U { int a; int b; int c[2]; }; > + > +constexpr int test1() { > + U u; > + u.a = 10; > + *&u.b = 20; // { dg-error "accessing" } > + return u.b; > +} > +constexpr int x1 = test1(); // { dg-message "in .constexpr. expansion" } > + > +constexpr int test2() { > + U u; > + u.a = 10; > + (0, u.b) = 20; // { dg-error "accessing" } > + return u.b; > +} > +constexpr int x2 = test2(); // { dg-message "in .constexpr. expansion" } > + > +constexpr int test3() { > + U u; > + u.a = 0; > + int* p = &u.b; > + p[u.a] = 10; // { dg-error "accessing" } > + return u.b; > +} > +constexpr int x3 = test3(); // { dg-message "in .constexpr. expansion" } > + > +constexpr int test4() { > + U u; > + u.a = 0; > + int* p = &u.b; > + u.a[p] = 10; // { dg-error "accessing" } > + return u.b; > +} > +constexpr int x4 = test4(); // { dg-message "in .constexpr. expansion" } > + > +struct S { U u[10]; }; > +constexpr int test5() { > + S s; > + s.u[4].a = 10; > + 6[s.u].b = 15; > + return 4[s.u].a + s.u[6].b; > +} > +static_assert(test5() == 25); > + > +constexpr int test6() { > + U u; > + u.a = 5; > + u.c[0] = 3; > + 1[u.c] = 8; > + return 1[u.c] + u.c[0]; > +} > +static_assert(test6() == 11); > + > +constexpr int test7() { > + U u {}; // value initialisation initialises first member > + int* p = &u.a; > + *p = 10; > + return *p; > +} > +constexpr int x7 = test7(); > + > +constexpr int test8() { > + U u; // default initialisation leaves no member initialised > + int* p = &u.a; > + *p = 10; // { dg-error "accessing" } > + return *p; > +} > +constexpr int x8 = test8(); // { dg-message "in .constexpr. expansion" } > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union6.C b/gcc/testsuite/g++.dg/cpp2a/constexpr-union6.C > new file mode 100644 > index 00000000000..7c37716892c > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union6.C > @@ -0,0 +1,43 @@ > +// { dg-do compile { target c++20 } } > +// PR c++/102286 > + > +#include "construct_at.h" > + > +struct S { const int a; int b; }; > +union U { int k; S s; }; > + > +constexpr int test1() { > + U u {}; > + std::construct_at(&u.s, S{ 1, 2 }); > + return u.s.b; > +} > +static_assert(test1() == 2); > + > +constexpr int test2() { > + U u {}; > + int* p = &u.s.b; > + std::construct_at(p, 5); // { dg-message "in .constexpr. expansion" } > + return u.s.b; > +} > +constexpr int x2 = test2(); // { dg-message "in .constexpr. expansion" } > + > +struct S2 { int a; int b; }; > +union U2 { int k; S2 s; }; > +constexpr int test3() { > + U2 u; > + int* p = &u.s.b; > + std::construct_at(p, 8); // { dg-message "in .constexpr. expansion" } > + return u.s.b; > +}; > +constexpr int x3 = test3(); // { dg-message "in .constexpr. expansion" } > + > +constexpr int test4() { > + union { > + int data[1]; > + } u; > + std::construct_at(u.data, 0); // { dg-message "in .constexpr. expansion" } > + return 0; > +} > +constexpr int x4 = test4(); // { dg-message "in .constexpr. expansion" } > + > +// { dg-error "accessing uninitialized member" "" { target *-*-* } 0 } > -- > 2.41.0 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286] 2023-09-23 0:38 ` [PATCH v4] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286] 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 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Wakely @ 2023-09-23 6:40 UTC (permalink / raw) To: Nathaniel Shead; +Cc: Jason Merrill, libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 44068 bytes --] On Sat, 23 Sept 2023, 01:39 Nathaniel Shead via Libstdc++, < libstdc++@gcc.gnu.org> wrote: > Now that bootstrap has finished, I have gotten regressions in the > following libstdc++ tests: > > Running libstdc++:libstdc++-dg/conformance.exp ... > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++23 (test for excess > errors) > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++26 (test for excess > errors) > FAIL: 20_util/variant/constexpr.cc -std=gnu++20 (test for excess errors) > FAIL: 20_util/variant/constexpr.cc -std=gnu++26 (test for excess errors) > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++20 (test > for excess errors) > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++26 (test > for excess errors) > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++20 (test > for excess errors) > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++26 (test > for excess errors) > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > -std=gnu++20 (test for excess errors) > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > -std=gnu++26 (test for excess errors) > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++20 > (test for excess errors) > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++26 > (test for excess errors) > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++23 (test for excess > errors) > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++23 compilation > failed to produce executable > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++26 (test for excess > errors) > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++26 compilation > failed to produce executable > > On investigation though it looks like the issue might be with libstdc++ > rather than the patch itself; running the failing tests using clang with > libstdc++ also produces similar errors, and my reading of the code > suggests that this is correct. > > What's the way forward here? Should I look at creating a patch to fix > the libstdc++ issues before resubmitting this patch for the C++ > frontend? Or should I submit a version of this patch without the > `std::construct_at` changes and wait till libstdc++ gets fixed for that? > I think we should fix libstdc++. There are probably only a few places that need a fix, which cause all those failures. I can help with those fixes. I'll look into it after the weekend. > On Sat, Sep 23, 2023 at 01:01:20AM +1000, Nathaniel Shead wrote: > > On Fri, Sep 22, 2023 at 02:21:15PM +0100, Jason Merrill wrote: > > > On 9/21/23 09:41, Nathaniel Shead wrote: > > > > I've updated the error messages, and also fixed another bug I found > > > > while retesting (value-initialised unions weren't considered to have > any > > > > active member yet). > > > > > > > > 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, which catches some additional mistakes pre-C++20. > > > > > > > > PR c++/101631 > > > > > > > > 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_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/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. > > > > > > > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > > > > --- > > > > gcc/cp/call.cc | 11 +- > > > > gcc/cp/class.cc | 8 ++ > > > > gcc/cp/constexpr.cc | 129 > +++++++++++++----- > > > > gcc/cp/cp-tree.h | 1 + > > > > .../g++.dg/cpp1y/constexpr-89336-3.C | 2 +- > > > > gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C | 13 ++ > > > > gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C | 30 ++++ > > > > gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C | 45 ++++++ > > > > gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C | 29 ++++ > > > > gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C | 71 ++++++++++ > > > > 10 files changed, 296 insertions(+), 43 deletions(-) > > > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C > > > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C > > > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C > > > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C > > > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C > > > > > > > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc > > > > index e8dafbd8ba6..c1fb8807d3f 100644 > > > > --- a/gcc/cp/call.cc > > > > +++ b/gcc/cp/call.cc > > > > @@ -10330,10 +10330,7 @@ build_over_call (struct z_candidate *cand, > int flags, tsubst_flags_t complain) > > > > && DECL_OVERLOADED_OPERATOR_IS (fn, NOP_EXPR) > > > > && trivial_fn_p (fn)) > > > > { > > > > - /* Don't use cp_build_fold_indirect_ref, op= returns an > lvalue even if > > > > - the object argument isn't one. */ > > > > - tree to = cp_build_indirect_ref (input_location, argarray[0], > > > > - RO_ARROW, complain); > > > > + tree to = cp_build_fold_indirect_ref (argarray[0]); > > > > tree type = TREE_TYPE (to); > > > > tree as_base = CLASSTYPE_AS_BASE (type); > > > > tree arg = argarray[1]; > > > > @@ -10341,7 +10338,11 @@ build_over_call (struct z_candidate *cand, > int flags, tsubst_flags_t complain) > > > > if (is_really_empty_class (type, /*ignore_vptr*/true)) > > > > { > > > > - /* Avoid copying empty classes. */ > > > > + /* Avoid copying empty classes, but ensure op= returns an lvalue > even > > > > + if the object argument isn't one. This isn't needed in other > cases > > > > + since MODIFY_EXPR is always considered an lvalue. */ > > > > + to = cp_build_addr_expr (to, tf_none); > > > > + to = cp_build_indirect_ref (input_location, to, RO_ARROW, > complain); > > > > val = build2 (COMPOUND_EXPR, type, arg, to); > > > > suppress_warning (val, OPT_Wunused); > > > > } > > > > diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc > > > > index b71333af1f8..e31aeb8e68b 100644 > > > > --- a/gcc/cp/class.cc > > > > +++ b/gcc/cp/class.cc > > > > @@ -5688,6 +5688,14 @@ type_has_virtual_destructor (tree type) > > > > return (dtor && DECL_VIRTUAL_P (dtor)); > > > > } > > > > +/* True iff class TYPE has a non-deleted trivial default > > > > + constructor. */ > > > > + > > > > +bool type_has_non_deleted_trivial_default_ctor (tree type) > > > > +{ > > > > + return TYPE_HAS_TRIVIAL_DFLT (type) && locate_ctor (type); > > > > +} > > > > + > > > > /* Returns true iff T, a class, has a move-assignment or > > > > move-constructor. Does not lazily declare either. > > > > If USER_P is false, any move function will do. If it is true, > the > > > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > > > > index a673a6022f1..5e50fc2c792 100644 > > > > --- a/gcc/cp/constexpr.cc > > > > +++ b/gcc/cp/constexpr.cc > > > > @@ -3146,40 +3146,34 @@ cxx_eval_call_expression (const > constexpr_ctx *ctx, tree t, > > > > cxx_set_object_constness (ctx, new_obj, > /*readonly_p=*/false, > > > > non_constant_p, overflow_p); > > > > + /* If this is a constructor, we are beginning the lifetime of the > > > > + object we are initializing. */ > > > > + if (new_obj > > > > + && DECL_CONSTRUCTOR_P (fun) > > > > + && TREE_CODE (new_obj) == COMPONENT_REF > > > > + && TREE_CODE (TREE_TYPE (TREE_OPERAND (new_obj, 0))) == > UNION_TYPE) > > > > + { > > > > + tree activate = build2 (INIT_EXPR, TREE_TYPE (new_obj), > > > > + new_obj, > > > > + build_constructor (TREE_TYPE > (new_obj), > > > > + NULL)); > > > > + cxx_eval_constant_expression (ctx, activate, > > > > + lval, non_constant_p, > overflow_p); > > > > + ggc_free (activate); > > > > + } > > > > + > > > > tree jump_target = NULL_TREE; > > > > cxx_eval_constant_expression (&call_ctx, body, > > > > vc_discard, > non_constant_p, overflow_p, > > > > &jump_target); > > > > if (DECL_CONSTRUCTOR_P (fun)) > > > > - { > > > > - /* This can be null for a subobject constructor call, in > > > > - which case what we care about is the initialization > > > > - side-effects rather than the value. We could get at the > > > > - value by evaluating *this, but we don't bother; there's > > > > - no need to put such a call in the hash table. */ > > > > - result = lval ? ctx->object : ctx->ctor; > > > > - > > > > - /* If we've just evaluated a subobject constructor call for > an > > > > - empty union member, it might not have produced a side > effect > > > > - that actually activated the union member. So produce > such a > > > > - side effect now to ensure the union appears initialized. > */ > > > > - if (!result && new_obj > > > > - && TREE_CODE (new_obj) == COMPONENT_REF > > > > - && TREE_CODE (TREE_TYPE > > > > - (TREE_OPERAND (new_obj, 0))) == UNION_TYPE > > > > - && is_really_empty_class (TREE_TYPE (new_obj), > > > > - /*ignore_vptr*/false)) > > > > - { > > > > - tree activate = build2 (MODIFY_EXPR, TREE_TYPE (new_obj), > > > > - new_obj, > > > > - build_constructor (TREE_TYPE > (new_obj), > > > > - NULL)); > > > > - cxx_eval_constant_expression (ctx, activate, lval, > > > > - non_constant_p, > overflow_p); > > > > - ggc_free (activate); > > > > - } > > > > - } > > > > + /* This can be null for a subobject constructor call, in > > > > + which case what we care about is the initialization > > > > + side-effects rather than the value. We could get at the > > > > + value by evaluating *this, but we don't bother; there's > > > > + no need to put such a call in the hash table. */ > > > > + result = lval ? ctx->object : ctx->ctor; > > > > else if (VOID_TYPE_P (TREE_TYPE (res))) > > > > result = void_node; > > > > else > > > > @@ -6127,6 +6121,14 @@ cxx_eval_store_expression (const > constexpr_ctx *ctx, tree t, > > > > mutable_p) > > > > && const_object_being_modified == NULL_TREE) > > > > const_object_being_modified = probe; > > > > + > > > > + /* Track named member accesses for unions to validate > modifications > > > > + that change active member. */ > > > > + if (!evaluated && TREE_CODE (probe) == COMPONENT_REF) > > > > + vec_safe_push (refs, probe); > > > > + else > > > > + vec_safe_push (refs, NULL_TREE); > > > > + > > > > vec_safe_push (refs, elt); > > > > vec_safe_push (refs, TREE_TYPE (probe)); > > > > probe = ob; > > > > @@ -6135,6 +6137,7 @@ cxx_eval_store_expression (const constexpr_ctx > *ctx, tree t, > > > > case REALPART_EXPR: > > > > gcc_assert (probe == target); > > > > + vec_safe_push (refs, NULL_TREE); > > > > vec_safe_push (refs, probe); > > > > vec_safe_push (refs, TREE_TYPE (probe)); > > > > probe = TREE_OPERAND (probe, 0); > > > > @@ -6142,6 +6145,7 @@ cxx_eval_store_expression (const constexpr_ctx > *ctx, tree t, > > > > case IMAGPART_EXPR: > > > > gcc_assert (probe == target); > > > > + vec_safe_push (refs, NULL_TREE); > > > > vec_safe_push (refs, probe); > > > > vec_safe_push (refs, TREE_TYPE (probe)); > > > > probe = TREE_OPERAND (probe, 0); > > > > @@ -6230,6 +6234,7 @@ cxx_eval_store_expression (const constexpr_ctx > *ctx, tree t, > > > > enum tree_code code = TREE_CODE (type); > > > > tree reftype = refs->pop(); > > > > tree index = refs->pop(); > > > > + bool is_access_expr = refs->pop() != NULL_TREE; > > > > if (code == COMPLEX_TYPE) > > > > { > > > > @@ -6270,21 +6275,72 @@ cxx_eval_store_expression (const > constexpr_ctx *ctx, tree t, > > > > type = reftype; > > > > - if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp) > > > > - && CONSTRUCTOR_ELT (*valp, 0)->index != index) > > > > + if (code == UNION_TYPE > > > > + && TREE_CODE (t) == MODIFY_EXPR > > > > + && (CONSTRUCTOR_NELTS (*valp) == 0 > > > > + || CONSTRUCTOR_ELT (*valp, 0)->index != index)) > > > > { > > > > - if (cxx_dialect < cxx20) > > > > + /* Probably a change of active member for a union. Ensure that > > > > + this is valid. */ > > > > + no_zero_init = true; > > > > + > > > > + tree init = *valp; > > > > + if (!CONSTRUCTOR_NO_CLEARING (*valp) && CONSTRUCTOR_NELTS > (*valp) == 0) > > > > + /* This is a value-initialized union, process the > initialization > > > > + to ensure that the active member is properly set. */ > > > > + init = build_value_init (TREE_TYPE (*valp), > tf_warning_or_error); > > > > + > > > > + bool has_active_member = CONSTRUCTOR_NELTS (init) != 0; > > > > + tree inner = strip_array_types (reftype); > > > > + > > > > + if (has_active_member && CONSTRUCTOR_ELT (init, 0)->index == > index) > > > > + /* After handling value-initialization, this wasn't actually a > > > > + change of active member like we initially thought. */ > > > > + no_zero_init = false; > > > > + else if (has_active_member && cxx_dialect < cxx20) > > > > { > > > > if (!ctx->quiet) > > > > error_at (cp_expr_loc_or_input_loc (t), > > > > "change of the active member of a union " > > > > - "from %qD to %qD", > > > > - CONSTRUCTOR_ELT (*valp, 0)->index, > > > > + "from %qD to %qD is not a constant expression " > > > > + "before C++20", > > > > + CONSTRUCTOR_ELT (init, 0)->index, > > > > index); > > > > *non_constant_p = true; > > > > } > > > > - else if (TREE_CODE (t) == MODIFY_EXPR > > > > - && CONSTRUCTOR_NO_CLEARING (*valp)) > > > > + else if (!is_access_expr > > > > + || (CLASS_TYPE_P (inner) > > > > + && !type_has_non_deleted_trivial_default_ctor > (inner))) > > > > + { > > > > + /* Diagnose changing active union member after initialization > > > > + without a valid member access expression, as described in > > > > + [class.union.general] p5. */ > > > > + if (!ctx->quiet) > > > > + { > > > > + if (has_active_member) > > > > + error_at (cp_expr_loc_or_input_loc (t), > > > > + "accessing %qD member instead of initialized > " > > > > + "%qD member in constant expression", > > > > + index, CONSTRUCTOR_ELT (init, 0)->index); > > > > + else > > > > + error_at (cp_expr_loc_or_input_loc (t), > > > > + "accessing uninitialized member %qD", > > > > + index); > > > > + if (is_access_expr) > > > > + inform (DECL_SOURCE_LOCATION (index), > > > > + "%qD does not implicitly begin its lifetime " > > > > + "because %qT does not have a non-deleted " > > > > + "trivial default constructor", > > > > + index, inner); > > > > > > It now occurs to me that this should suggest using placement new > instead. > > > And that we should test using placement new to begin the lifetime of > such > > > members. > > > > > > Jason > > > > > > > Good point. On testing placement new I found I wasn't checking for > > indirect change of active member during initialisation either, but this > > was pretty trivial to add. How does this look? > > > > Bootstrap + regtest ongoing, but dg.exp completed with no errors. I'm > > away from my computer next week so I thought I'd send this through now > > anyway. > > > > -- >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, 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_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/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> > > --- > > gcc/cp/call.cc | 11 +- > > gcc/cp/class.cc | 8 ++ > > gcc/cp/constexpr.cc | 135 +++++++++++++----- > > gcc/cp/cp-tree.h | 1 + > > .../g++.dg/cpp1y/constexpr-89336-3.C | 2 +- > > gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C | 13 ++ > > gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C | 30 ++++ > > gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C | 45 ++++++ > > gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C | 29 ++++ > > gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C | 71 +++++++++ > > gcc/testsuite/g++.dg/cpp2a/constexpr-union6.C | 43 ++++++ > > 11 files changed, 345 insertions(+), 43 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union6.C > > > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc > > index e8dafbd8ba6..c1fb8807d3f 100644 > > --- a/gcc/cp/call.cc > > +++ b/gcc/cp/call.cc > > @@ -10330,10 +10330,7 @@ build_over_call (struct z_candidate *cand, int > flags, tsubst_flags_t complain) > > && DECL_OVERLOADED_OPERATOR_IS (fn, NOP_EXPR) > > && trivial_fn_p (fn)) > > { > > - /* Don't use cp_build_fold_indirect_ref, op= returns an lvalue > even if > > - the object argument isn't one. */ > > - tree to = cp_build_indirect_ref (input_location, argarray[0], > > - RO_ARROW, complain); > > + tree to = cp_build_fold_indirect_ref (argarray[0]); > > tree type = TREE_TYPE (to); > > tree as_base = CLASSTYPE_AS_BASE (type); > > tree arg = argarray[1]; > > @@ -10341,7 +10338,11 @@ build_over_call (struct z_candidate *cand, int > flags, tsubst_flags_t complain) > > > > if (is_really_empty_class (type, /*ignore_vptr*/true)) > > { > > - /* Avoid copying empty classes. */ > > + /* Avoid copying empty classes, but ensure op= returns an lvalue > even > > + if the object argument isn't one. This isn't needed in other > cases > > + since MODIFY_EXPR is always considered an lvalue. */ > > + to = cp_build_addr_expr (to, tf_none); > > + to = cp_build_indirect_ref (input_location, to, RO_ARROW, > complain); > > val = build2 (COMPOUND_EXPR, type, arg, to); > > suppress_warning (val, OPT_Wunused); > > } > > diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc > > index b71333af1f8..e31aeb8e68b 100644 > > --- a/gcc/cp/class.cc > > +++ b/gcc/cp/class.cc > > @@ -5688,6 +5688,14 @@ type_has_virtual_destructor (tree type) > > return (dtor && DECL_VIRTUAL_P (dtor)); > > } > > > > +/* True iff class TYPE has a non-deleted trivial default > > + constructor. */ > > + > > +bool type_has_non_deleted_trivial_default_ctor (tree type) > > +{ > > + return TYPE_HAS_TRIVIAL_DFLT (type) && locate_ctor (type); > > +} > > + > > /* Returns true iff T, a class, has a move-assignment or > > move-constructor. Does not lazily declare either. > > If USER_P is false, any move function will do. If it is true, the > > diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc > > index a673a6022f1..8e6f60d03d2 100644 > > --- a/gcc/cp/constexpr.cc > > +++ b/gcc/cp/constexpr.cc > > @@ -3146,40 +3146,34 @@ cxx_eval_call_expression (const constexpr_ctx > *ctx, tree t, > > cxx_set_object_constness (ctx, new_obj, /*readonly_p=*/false, > > non_constant_p, overflow_p); > > > > + /* If this is a constructor, we are beginning the lifetime of the > > + object we are initializing. */ > > + if (new_obj > > + && DECL_CONSTRUCTOR_P (fun) > > + && TREE_CODE (new_obj) == COMPONENT_REF > > + && TREE_CODE (TREE_TYPE (TREE_OPERAND (new_obj, 0))) == > UNION_TYPE) > > + { > > + tree activate = build2 (INIT_EXPR, TREE_TYPE (new_obj), > > + new_obj, > > + build_constructor (TREE_TYPE > (new_obj), > > + NULL)); > > + cxx_eval_constant_expression (ctx, activate, > > + lval, non_constant_p, > overflow_p); > > + ggc_free (activate); > > + } > > + > > tree jump_target = NULL_TREE; > > cxx_eval_constant_expression (&call_ctx, body, > > vc_discard, non_constant_p, > overflow_p, > > &jump_target); > > > > if (DECL_CONSTRUCTOR_P (fun)) > > - { > > - /* This can be null for a subobject constructor call, in > > - which case what we care about is the initialization > > - side-effects rather than the value. We could get at the > > - value by evaluating *this, but we don't bother; there's > > - no need to put such a call in the hash table. */ > > - result = lval ? ctx->object : ctx->ctor; > > - > > - /* If we've just evaluated a subobject constructor call for > an > > - empty union member, it might not have produced a side > effect > > - that actually activated the union member. So produce > such a > > - side effect now to ensure the union appears initialized. > */ > > - if (!result && new_obj > > - && TREE_CODE (new_obj) == COMPONENT_REF > > - && TREE_CODE (TREE_TYPE > > - (TREE_OPERAND (new_obj, 0))) == UNION_TYPE > > - && is_really_empty_class (TREE_TYPE (new_obj), > > - /*ignore_vptr*/false)) > > - { > > - tree activate = build2 (MODIFY_EXPR, TREE_TYPE (new_obj), > > - new_obj, > > - build_constructor (TREE_TYPE > (new_obj), > > - NULL)); > > - cxx_eval_constant_expression (ctx, activate, lval, > > - non_constant_p, > overflow_p); > > - ggc_free (activate); > > - } > > - } > > + /* This can be null for a subobject constructor call, in > > + which case what we care about is the initialization > > + side-effects rather than the value. We could get at the > > + value by evaluating *this, but we don't bother; there's > > + no need to put such a call in the hash table. */ > > + result = lval ? ctx->object : ctx->ctor; > > else if (VOID_TYPE_P (TREE_TYPE (res))) > > result = void_node; > > else > > @@ -6127,6 +6121,14 @@ cxx_eval_store_expression (const constexpr_ctx > *ctx, tree t, > > mutable_p) > > && const_object_being_modified == NULL_TREE) > > const_object_being_modified = probe; > > + > > + /* Track named member accesses for unions to validate > modifications > > + that change active member. */ > > + if (!evaluated && TREE_CODE (probe) == COMPONENT_REF) > > + vec_safe_push (refs, probe); > > + else > > + vec_safe_push (refs, NULL_TREE); > > + > > vec_safe_push (refs, elt); > > vec_safe_push (refs, TREE_TYPE (probe)); > > probe = ob; > > @@ -6135,6 +6137,7 @@ cxx_eval_store_expression (const constexpr_ctx > *ctx, tree t, > > > > case REALPART_EXPR: > > gcc_assert (probe == target); > > + vec_safe_push (refs, NULL_TREE); > > vec_safe_push (refs, probe); > > vec_safe_push (refs, TREE_TYPE (probe)); > > probe = TREE_OPERAND (probe, 0); > > @@ -6142,6 +6145,7 @@ cxx_eval_store_expression (const constexpr_ctx > *ctx, tree t, > > > > case IMAGPART_EXPR: > > gcc_assert (probe == target); > > + vec_safe_push (refs, NULL_TREE); > > vec_safe_push (refs, probe); > > vec_safe_push (refs, TREE_TYPE (probe)); > > probe = TREE_OPERAND (probe, 0); > > @@ -6230,6 +6234,7 @@ cxx_eval_store_expression (const constexpr_ctx > *ctx, tree t, > > enum tree_code code = TREE_CODE (type); > > tree reftype = refs->pop(); > > tree index = refs->pop(); > > + bool is_access_expr = refs->pop() != NULL_TREE; > > > > if (code == COMPLEX_TYPE) > > { > > @@ -6270,21 +6275,78 @@ cxx_eval_store_expression (const constexpr_ctx > *ctx, tree t, > > > > type = reftype; > > > > - if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp) > > - && CONSTRUCTOR_ELT (*valp, 0)->index != index) > > + /* Check for implicit change of active member for a union. */ > > + if (code == UNION_TYPE > > + && (TREE_CODE (t) == MODIFY_EXPR > > + /* Also check if initializations have implicit change of > active > > + member earlier up the access chain. */ > > + || !refs->is_empty()) > > + && (CONSTRUCTOR_NELTS (*valp) == 0 > > + || CONSTRUCTOR_ELT (*valp, 0)->index != index)) > > { > > - if (cxx_dialect < cxx20) > > + no_zero_init = true; > > + > > + tree init = *valp; > > + if (TREE_CODE (t) == MODIFY_EXPR > > + && !CONSTRUCTOR_NO_CLEARING (*valp) > > + && CONSTRUCTOR_NELTS (*valp) == 0) > > + /* Modifying a value-initialized union, process the > initialization > > + to ensure that the active member is properly set. */ > > + init = build_value_init (TREE_TYPE (*valp), > tf_warning_or_error); > > + > > + bool has_active_member = CONSTRUCTOR_NELTS (init) != 0; > > + tree inner = strip_array_types (reftype); > > + > > + if (has_active_member && CONSTRUCTOR_ELT (init, 0)->index == > index) > > + /* After handling value-initialization, this wasn't actually a > > + change of active member like we initially thought. */ > > + no_zero_init = false; > > + else if (has_active_member && cxx_dialect < cxx20) > > { > > if (!ctx->quiet) > > error_at (cp_expr_loc_or_input_loc (t), > > "change of the active member of a union " > > - "from %qD to %qD", > > - CONSTRUCTOR_ELT (*valp, 0)->index, > > + "from %qD to %qD is not a constant expression " > > + "before C++20", > > + CONSTRUCTOR_ELT (init, 0)->index, > > index); > > *non_constant_p = true; > > } > > - else if (TREE_CODE (t) == MODIFY_EXPR > > - && CONSTRUCTOR_NO_CLEARING (*valp)) > > + else if (!is_access_expr > > + || (TREE_CODE (t) == MODIFY_EXPR > > + && CLASS_TYPE_P (inner) > > + && !type_has_non_deleted_trivial_default_ctor > (inner))) > > + { > > + /* Diagnose changing active union member after initialization > > + without a valid member access expression, as described in > > + [class.union.general] p5. */ > > + if (!ctx->quiet) > > + { > > + if (has_active_member) > > + error_at (cp_expr_loc_or_input_loc (t), > > + "accessing %qD member instead of initialized > " > > + "%qD member in constant expression", > > + index, CONSTRUCTOR_ELT (init, 0)->index); > > + else > > + error_at (cp_expr_loc_or_input_loc (t), > > + "accessing uninitialized member %qD", > > + index); > > + if (is_access_expr) > > + inform (DECL_SOURCE_LOCATION (index), > > + "%qD does not implicitly begin its lifetime " > > + "because %qT does not have a non-deleted " > > + "trivial default constructor, use " > > + "%<std::construct_at%> instead", > > + index, inner); > > + else > > + inform (DECL_SOURCE_LOCATION (index), > > + "initializing %qD requires a member access " > > + "expression as the left operand of the > assignment", > > + index); > > + } > > + *non_constant_p = true; > > + } > > + else if (has_active_member && CONSTRUCTOR_NO_CLEARING (init)) > > { > > /* Diagnose changing the active union member while the union > > is in the process of being initialized. */ > > @@ -6292,11 +6354,10 @@ cxx_eval_store_expression (const constexpr_ctx > *ctx, tree t, > > error_at (cp_expr_loc_or_input_loc (t), > > "change of the active member of a union " > > "from %qD to %qD during initialization", > > - CONSTRUCTOR_ELT (*valp, 0)->index, > > + CONSTRUCTOR_ELT (init, 0)->index, > > index); > > *non_constant_p = true; > > } > > - no_zero_init = true; > > } > > > > ctors.safe_push (valp); > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > > index fd6bf9fd4a0..40aa9f0be2b 100644 > > --- a/gcc/cp/cp-tree.h > > +++ b/gcc/cp/cp-tree.h > > @@ -6815,6 +6815,7 @@ extern bool > trivial_default_constructor_is_constexpr (tree); > > extern bool type_has_constexpr_default_constructor (tree); > > extern bool type_has_constexpr_destructor (tree); > > extern bool type_has_virtual_destructor (tree); > > +extern bool type_has_non_deleted_trivial_default_ctor (tree); > > extern bool classtype_has_move_assign_or_move_ctor_p (tree, bool > user_declared); > > extern bool classtype_has_non_deleted_move_ctor (tree); > > extern tree classtype_has_depr_implicit_copy (tree); > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C > b/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C > > index 9d370ddefcf..6a60966f6f8 100644 > > --- a/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C > > @@ -18,7 +18,7 @@ constexpr int > > bar () > > { > > union U { int a[5]; long b; }; > > - union V { union U u; short v; }; > > + union V { short v; union U u; }; > > V w {}; > > w.v = 5; > > w.u.a[3] = w.u.a[1] = w.v; // { dg-error "change of the > active member of a union from" "" { target c++17_down } } > > diff --git a/gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C > b/gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C > > new file mode 100644 > > index 00000000000..ff7ebf19f89 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C > > @@ -0,0 +1,13 @@ > > +// { dg-do compile { target c++14 } } > > + > > +union U { > > + int a; > > + float b; > > +}; > > + > > +constexpr bool foo() { > > + U u {}; > > + u.b = 1.0f; // { dg-error "change of the active member" "" { target > c++17_down } } > > + return u.b == 1.0f; > > +} > > +constexpr bool x = foo(); > > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C > b/gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C > > new file mode 100644 > > index 00000000000..1712395d7e7 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C > > @@ -0,0 +1,30 @@ > > +// PR c++/101631 > > +// { dg-do compile { target c++20 } } > > + > > +struct sso { > > + union { > > + int buf[10]; > > + int* alloc; > > + }; > > +}; > > + > > +constexpr bool direct() { > > + sso val; > > + val.alloc = nullptr; > > + val.buf[5] = 42; > > + return true; > > +} > > +constexpr bool ok = direct(); > > + > > + > > +constexpr void perform_assignment(int& left, int right) noexcept { > > + left = right; // { dg-error "accessing .+ member instead of > initialized" } > > +} > > + > > +constexpr bool indirect() { > > + sso val; > > + val.alloc = nullptr; > > + perform_assignment(val.buf[5], 42); // { dg-message "in .constexpr. > expansion" } > > + return true; > > +} > > +constexpr bool err = indirect(); // { dg-message "in .constexpr. > expansion" } > > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C > b/gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C > > new file mode 100644 > > index 00000000000..6d30bb2498f > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C > > @@ -0,0 +1,45 @@ > > +// { dg-do compile { target c++20 } } > > + > > +struct S > > +{ > > + union { > > + char buf[8]; > > + char* ptr; > > + }; > > + unsigned len; > > + > > + constexpr S(const char* s, unsigned n) > > + { > > + char* p; > > + if (n > 7) > > + p = ptr = new char[n+1]; > > + else > > + p = buf; > > + for (len = 0; len < n; ++len) > > + p[len] = s[len]; // { dg-error "accessing uninitialized member" } > > + p[len] = '\0'; > > + } > > + > > + constexpr ~S() > > + { > > + if (len > 7) > > + delete[] ptr; > > + } > > +}; > > + > > +constexpr bool test1() > > +{ > > + S s("test", 4); // { dg-message "in .constexpr. expansion" } > > + return true; > > +} > > + > > +constexpr bool a = test1(); // { dg-message "in .constexpr. expansion" > } > > + > > + > > +constexpr bool test2() > > +{ > > + S s("hello world", 11); > > + return true; > > +} > > + > > +constexpr bool b = test2(); > > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C > b/gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C > > new file mode 100644 > > index 00000000000..429ab203ac2 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C > > @@ -0,0 +1,29 @@ > > +// { dg-do compile { target c++20 } } > > + > > +// from [class.union.general] p5 > > + > > +union A { int x; int y[4]; }; > > +struct B { A a; }; > > +union C { B b; int k; }; > > +constexpr int f() { > > + C c; // does not start lifetime of any union member > > + c.b.a.y[3] = 4; // OK, S(c.b.a.y[3]) contains c.b and c.b.a.y; > > + // creates objects to hold union members c.b > and c.b.a.y > > + return c.b.a.y[3]; // OK, c.b.a.y refers to newly created object > (see [basic.life]) > > +} > > +constexpr int a = f(); > > + > > +struct X { const int a; int b; }; > > +union Y { X x; int k; };// { dg-message "does not implicitly begin its > lifetime" } > > +constexpr int g() { > > + Y y = { { 1, 2 } }; // OK, y.x is active union member ([class.mem]) > > + int n = y.x.a; > > + y.k = 4; // OK, ends lifetime of y.x, y.k is active > member of union > > + > > + y.x.b = n; // { dg-error "accessing .* member instead of > initialized .* member" } > > + // undefined behavior: y.x.b modified outside > its lifetime, > > + // S(y.x.b) is empty because X's default > constructor is deleted, > > + // so union member y.x's lifetime does not > implicitly start > > + return 0; > > +} > > +constexpr int b = g(); // { dg-message "in .constexpr. expansion" } > > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C > b/gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C > > new file mode 100644 > > index 00000000000..c94d7cc0fc9 > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C > > @@ -0,0 +1,71 @@ > > +// { dg-do compile { target c++20 } } > > + > > +union U { int a; int b; int c[2]; }; > > + > > +constexpr int test1() { > > + U u; > > + u.a = 10; > > + *&u.b = 20; // { dg-error "accessing" } > > + return u.b; > > +} > > +constexpr int x1 = test1(); // { dg-message "in .constexpr. expansion" > } > > + > > +constexpr int test2() { > > + U u; > > + u.a = 10; > > + (0, u.b) = 20; // { dg-error "accessing" } > > + return u.b; > > +} > > +constexpr int x2 = test2(); // { dg-message "in .constexpr. expansion" > } > > + > > +constexpr int test3() { > > + U u; > > + u.a = 0; > > + int* p = &u.b; > > + p[u.a] = 10; // { dg-error "accessing" } > > + return u.b; > > +} > > +constexpr int x3 = test3(); // { dg-message "in .constexpr. expansion" > } > > + > > +constexpr int test4() { > > + U u; > > + u.a = 0; > > + int* p = &u.b; > > + u.a[p] = 10; // { dg-error "accessing" } > > + return u.b; > > +} > > +constexpr int x4 = test4(); // { dg-message "in .constexpr. expansion" > } > > + > > +struct S { U u[10]; }; > > +constexpr int test5() { > > + S s; > > + s.u[4].a = 10; > > + 6[s.u].b = 15; > > + return 4[s.u].a + s.u[6].b; > > +} > > +static_assert(test5() == 25); > > + > > +constexpr int test6() { > > + U u; > > + u.a = 5; > > + u.c[0] = 3; > > + 1[u.c] = 8; > > + return 1[u.c] + u.c[0]; > > +} > > +static_assert(test6() == 11); > > + > > +constexpr int test7() { > > + U u {}; // value initialisation initialises first member > > + int* p = &u.a; > > + *p = 10; > > + return *p; > > +} > > +constexpr int x7 = test7(); > > + > > +constexpr int test8() { > > + U u; // default initialisation leaves no member initialised > > + int* p = &u.a; > > + *p = 10; // { dg-error "accessing" } > > + return *p; > > +} > > +constexpr int x8 = test8(); // { dg-message "in .constexpr. expansion" > } > > diff --git a/gcc/testsuite/g++.dg/cpp2a/constexpr-union6.C > b/gcc/testsuite/g++.dg/cpp2a/constexpr-union6.C > > new file mode 100644 > > index 00000000000..7c37716892c > > --- /dev/null > > +++ b/gcc/testsuite/g++.dg/cpp2a/constexpr-union6.C > > @@ -0,0 +1,43 @@ > > +// { dg-do compile { target c++20 } } > > +// PR c++/102286 > > + > > +#include "construct_at.h" > > + > > +struct S { const int a; int b; }; > > +union U { int k; S s; }; > > + > > +constexpr int test1() { > > + U u {}; > > + std::construct_at(&u.s, S{ 1, 2 }); > > + return u.s.b; > > +} > > +static_assert(test1() == 2); > > + > > +constexpr int test2() { > > + U u {}; > > + int* p = &u.s.b; > > + std::construct_at(p, 5); // { dg-message "in .constexpr. expansion" } > > + return u.s.b; > > +} > > +constexpr int x2 = test2(); // { dg-message "in .constexpr. expansion" > } > > + > > +struct S2 { int a; int b; }; > > +union U2 { int k; S2 s; }; > > +constexpr int test3() { > > + U2 u; > > + int* p = &u.s.b; > > + std::construct_at(p, 8); // { dg-message "in .constexpr. expansion" } > > + return u.s.b; > > +}; > > +constexpr int x3 = test3(); // { dg-message "in .constexpr. expansion" > } > > + > > +constexpr int test4() { > > + union { > > + int data[1]; > > + } u; > > + std::construct_at(u.data, 0); // { dg-message "in .constexpr. > expansion" } > > + return 0; > > +} > > +constexpr int x4 = test4(); // { dg-message "in .constexpr. expansion" > } > > + > > +// { dg-error "accessing uninitialized member" "" { target *-*-* } 0 } > > -- > > 2.41.0 > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] libstdc++: Ensure active union member is correctly set 2023-09-23 6:40 ` Jonathan Wakely @ 2023-09-23 7:30 ` Nathaniel Shead 2023-09-23 10:52 ` Jonathan Wakely 2023-09-27 14:13 ` Jonathan Wakely 0 siblings, 2 replies; 11+ messages in thread From: Nathaniel Shead @ 2023-09-23 7:30 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Jason Merrill, libstdc++, gcc-patches On Sat, Sep 23, 2023 at 07:40:48AM +0100, Jonathan Wakely wrote: > On Sat, 23 Sept 2023, 01:39 Nathaniel Shead via Libstdc++, < > libstdc++@gcc.gnu.org> wrote: > > > Now that bootstrap has finished, I have gotten regressions in the > > following libstdc++ tests: > > > > Running libstdc++:libstdc++-dg/conformance.exp ... > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++23 (test for excess > > errors) > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++26 (test for excess > > errors) > > FAIL: 20_util/variant/constexpr.cc -std=gnu++20 (test for excess errors) > > FAIL: 20_util/variant/constexpr.cc -std=gnu++26 (test for excess errors) > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++20 (test > > for excess errors) > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++26 (test > > for excess errors) > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++20 (test > > for excess errors) > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++26 (test > > for excess errors) > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > -std=gnu++20 (test for excess errors) > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > -std=gnu++26 (test for excess errors) > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++20 > > (test for excess errors) > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++26 > > (test for excess errors) > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++23 (test for excess > > errors) > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++23 compilation > > failed to produce executable > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++26 (test for excess > > errors) > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++26 compilation > > failed to produce executable > > > > On investigation though it looks like the issue might be with libstdc++ > > rather than the patch itself; running the failing tests using clang with > > libstdc++ also produces similar errors, and my reading of the code > > suggests that this is correct. > > > > What's the way forward here? Should I look at creating a patch to fix > > the libstdc++ issues before resubmitting this patch for the C++ > > frontend? Or should I submit a version of this patch without the > > `std::construct_at` changes and wait till libstdc++ gets fixed for that? > > > > I think we should fix libstdc++. There are probably only a few places that > need a fix, which cause all those failures. > > I can help with those fixes. I'll look into it after the weekend. > Thanks. I did end up getting a chance to look at it earlier today, and with the following patch I had no regressions when applying the frontend changes. Bootstrapped and regtested on x86_64-pc-linux-gnu. -- >8 -- This patch ensures that the union members for std::string and std::variant are always properly set when a change occurs. libstdc++-v3/ChangeLog: * include/bits/basic_string.h: (basic_string(basic_string&&)): Activate _M_local_buf when needed. (basic_string(basic_string&&, const _Alloc&)): Likewise. * include/bits/basic_string.tcc: (basic_string::swap): Likewise. * include/std/variant: (__detail::__variant::__construct_n): New. (__detail::_variant::__emplace): Use __construct_n. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- libstdc++-v3/include/bits/basic_string.h | 7 +++-- libstdc++-v3/include/bits/basic_string.tcc | 8 +++--- libstdc++-v3/include/std/variant | 32 ++++++++++++++++++++-- 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index 09fd62afa66..7c342879827 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -678,7 +678,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { if (__str._M_is_local()) { - traits_type::copy(_M_local_buf, __str._M_local_buf, + traits_type::copy(_M_use_local_data(), __str._M_local_buf, __str.length() + 1); } else @@ -691,7 +691,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 // basic_stringbuf relies on writing into unallocated capacity so // we mess up the contents if we put a '\0' in the string. _M_length(__str.length()); - __str._M_data(__str._M_local_data()); + __str._M_data(__str._M_use_local_data()); __str._M_set_length(0); } @@ -717,6 +717,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { if (__str._M_is_local()) { + _M_use_local_data(); traits_type::copy(_M_local_buf, __str._M_local_buf, __str.length() + 1); _M_length(__str.length()); @@ -728,7 +729,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 _M_data(__str._M_data()); _M_length(__str.length()); _M_capacity(__str._M_allocated_capacity); - __str._M_data(__str._M_local_buf); + __str._M_data(__str._M_use_local_data()); __str._M_set_length(0); } else diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc index 104a517f794..ee6e57da555 100644 --- a/libstdc++-v3/include/bits/basic_string.tcc +++ b/libstdc++-v3/include/bits/basic_string.tcc @@ -79,7 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } else if (__s.length()) { - traits_type::copy(_M_local_buf, __s._M_local_buf, + traits_type::copy(_M_use_local_data(), __s._M_local_buf, __s.length() + 1); _M_length(__s.length()); __s._M_set_length(0); @@ -87,7 +87,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } else if (length()) { - traits_type::copy(__s._M_local_buf, _M_local_buf, + traits_type::copy(__s._M_use_local_data(), _M_local_buf, length() + 1); __s._M_length(length()); _M_set_length(0); @@ -97,7 +97,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION else { const size_type __tmp_capacity = __s._M_allocated_capacity; - traits_type::copy(__s._M_local_buf, _M_local_buf, + traits_type::copy(__s._M_use_local_data(), _M_local_buf, length() + 1); _M_data(__s._M_data()); __s._M_data(__s._M_local_buf); @@ -108,7 +108,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const size_type __tmp_capacity = _M_allocated_capacity; if (__s._M_is_local()) { - traits_type::copy(_M_local_buf, __s._M_local_buf, + traits_type::copy(_M_use_local_data(), __s._M_local_buf, __s.length() + 1); __s._M_data(_M_data()); _M_data(_M_local_buf); diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index c0e41740dcf..7f24e760bb1 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -320,6 +320,33 @@ namespace __variant __get(_Variant&& __v) noexcept { return __variant::__get_n<_Np>(std::forward<_Variant>(__v)._M_u); } + // Gets the _Uninitialized to construct into for __u. + template<size_t _Np, typename _Union> + constexpr decltype(auto) + __construct_n(_Union& __u) noexcept + { + if constexpr (_Np == 0) + return &__u._M_first; + else if constexpr (_Np == 1) + { + std::_Construct(&__u._M_rest); + return &__u._M_rest._M_first; + } + else if constexpr (_Np == 2) + { + std::_Construct(&__u._M_rest); + std::_Construct(&__u._M_rest._M_rest); + return &__u._M_rest._M_rest._M_first; + } + else + { + std::_Construct(&__u._M_rest); + std::_Construct(&__u._M_rest._M_rest); + std::_Construct(&__u._M_rest._M_rest._M_rest); + return __variant::__construct_n<_Np - 3>(__u._M_rest._M_rest._M_rest); + } + } + template<typename... _Types> struct _Traits { @@ -536,8 +563,9 @@ namespace __variant __emplace(_Variant_storage<_Triv, _Types...>& __v, _Args&&... __args) { __v._M_reset(); - auto* __addr = std::__addressof(__variant::__get_n<_Np>(__v._M_u)); - std::_Construct(__addr, std::forward<_Args>(__args)...); + auto* __addr = __variant::__construct_n<_Np>(__v._M_u); + std::_Construct(__addr, in_place_index<0>, + std::forward<_Args>(__args)...); // Construction didn't throw, so can set the new index now: __v._M_index = _Np; } -- 2.41.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libstdc++: Ensure active union member is correctly set 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 1 sibling, 0 replies; 11+ messages in thread From: Jonathan Wakely @ 2023-09-23 10:52 UTC (permalink / raw) To: Nathaniel Shead; +Cc: Jason Merrill, libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 9471 bytes --] On Sat, 23 Sept 2023, 08:30 Nathaniel Shead, <nathanieloshead@gmail.com> wrote: > On Sat, Sep 23, 2023 at 07:40:48AM +0100, Jonathan Wakely wrote: > > On Sat, 23 Sept 2023, 01:39 Nathaniel Shead via Libstdc++, < > > libstdc++@gcc.gnu.org> wrote: > > > > > Now that bootstrap has finished, I have gotten regressions in the > > > following libstdc++ tests: > > > > > > Running libstdc++:libstdc++-dg/conformance.exp ... > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++23 (test for excess > > > errors) > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++26 (test for excess > > > errors) > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++20 (test for excess > errors) > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++26 (test for excess > errors) > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++20 (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++26 (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++20 > (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++26 > (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > -std=gnu++20 (test for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > -std=gnu++26 (test for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++20 > > > (test for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++26 > > > (test for excess errors) > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++23 (test for excess > > > errors) > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++23 compilation > > > failed to produce executable > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++26 (test for excess > > > errors) > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++26 compilation > > > failed to produce executable > > > > > > On investigation though it looks like the issue might be with libstdc++ > > > rather than the patch itself; running the failing tests using clang > with > > > libstdc++ also produces similar errors, and my reading of the code > > > suggests that this is correct. > > > > > > What's the way forward here? Should I look at creating a patch to fix > > > the libstdc++ issues before resubmitting this patch for the C++ > > > frontend? Or should I submit a version of this patch without the > > > `std::construct_at` changes and wait till libstdc++ gets fixed for > that? > > > > > > > I think we should fix libstdc++. There are probably only a few places > that > > need a fix, which cause all those failures. > > > > I can help with those fixes. I'll look into it after the weekend. > > > > Thanks. I did end up getting a chance to look at it earlier today, and > with the following patch I had no regressions when applying the frontend > changes. Bootstrapped and regtested on x86_64-pc-linux-gnu. > Nice, thanks! This looks good at first glance, I'll review it properly ASAP. > -- >8 -- > > This patch ensures that the union members for std::string and > std::variant are always properly set when a change occurs. > > libstdc++-v3/ChangeLog: > > * include/bits/basic_string.h: (basic_string(basic_string&&)): > Activate _M_local_buf when needed. > (basic_string(basic_string&&, const _Alloc&)): Likewise. > * include/bits/basic_string.tcc: (basic_string::swap): Likewise. > * include/std/variant: (__detail::__variant::__construct_n): New. > (__detail::_variant::__emplace): Use __construct_n. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > libstdc++-v3/include/bits/basic_string.h | 7 +++-- > libstdc++-v3/include/bits/basic_string.tcc | 8 +++--- > libstdc++-v3/include/std/variant | 32 ++++++++++++++++++++-- > 3 files changed, 38 insertions(+), 9 deletions(-) > > diff --git a/libstdc++-v3/include/bits/basic_string.h > b/libstdc++-v3/include/bits/basic_string.h > index 09fd62afa66..7c342879827 100644 > --- a/libstdc++-v3/include/bits/basic_string.h > +++ b/libstdc++-v3/include/bits/basic_string.h > @@ -678,7 +678,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > { > if (__str._M_is_local()) > { > - traits_type::copy(_M_local_buf, __str._M_local_buf, > + traits_type::copy(_M_use_local_data(), __str._M_local_buf, > __str.length() + 1); > } > else > @@ -691,7 +691,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > // basic_stringbuf relies on writing into unallocated capacity so > // we mess up the contents if we put a '\0' in the string. > _M_length(__str.length()); > - __str._M_data(__str._M_local_data()); > + __str._M_data(__str._M_use_local_data()); > __str._M_set_length(0); > } > > @@ -717,6 +717,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > { > if (__str._M_is_local()) > { > + _M_use_local_data(); > traits_type::copy(_M_local_buf, __str._M_local_buf, > __str.length() + 1); > _M_length(__str.length()); > @@ -728,7 +729,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > _M_data(__str._M_data()); > _M_length(__str.length()); > _M_capacity(__str._M_allocated_capacity); > - __str._M_data(__str._M_local_buf); > + __str._M_data(__str._M_use_local_data()); > __str._M_set_length(0); > } > else > diff --git a/libstdc++-v3/include/bits/basic_string.tcc > b/libstdc++-v3/include/bits/basic_string.tcc > index 104a517f794..ee6e57da555 100644 > --- a/libstdc++-v3/include/bits/basic_string.tcc > +++ b/libstdc++-v3/include/bits/basic_string.tcc > @@ -79,7 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > else if (__s.length()) > { > - traits_type::copy(_M_local_buf, __s._M_local_buf, > + traits_type::copy(_M_use_local_data(), __s._M_local_buf, > __s.length() + 1); > _M_length(__s.length()); > __s._M_set_length(0); > @@ -87,7 +87,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > else if (length()) > { > - traits_type::copy(__s._M_local_buf, _M_local_buf, > + traits_type::copy(__s._M_use_local_data(), _M_local_buf, > length() + 1); > __s._M_length(length()); > _M_set_length(0); > @@ -97,7 +97,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > else > { > const size_type __tmp_capacity = __s._M_allocated_capacity; > - traits_type::copy(__s._M_local_buf, _M_local_buf, > + traits_type::copy(__s._M_use_local_data(), _M_local_buf, > length() + 1); > _M_data(__s._M_data()); > __s._M_data(__s._M_local_buf); > @@ -108,7 +108,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > const size_type __tmp_capacity = _M_allocated_capacity; > if (__s._M_is_local()) > { > - traits_type::copy(_M_local_buf, __s._M_local_buf, > + traits_type::copy(_M_use_local_data(), __s._M_local_buf, > __s.length() + 1); > __s._M_data(_M_data()); > _M_data(_M_local_buf); > diff --git a/libstdc++-v3/include/std/variant > b/libstdc++-v3/include/std/variant > index c0e41740dcf..7f24e760bb1 100644 > --- a/libstdc++-v3/include/std/variant > +++ b/libstdc++-v3/include/std/variant > @@ -320,6 +320,33 @@ namespace __variant > __get(_Variant&& __v) noexcept > { return __variant::__get_n<_Np>(std::forward<_Variant>(__v)._M_u); } > > + // Gets the _Uninitialized to construct into for __u. > + template<size_t _Np, typename _Union> > + constexpr decltype(auto) > + __construct_n(_Union& __u) noexcept > + { > + if constexpr (_Np == 0) > + return &__u._M_first; > + else if constexpr (_Np == 1) > + { > + std::_Construct(&__u._M_rest); > + return &__u._M_rest._M_first; > + } > + else if constexpr (_Np == 2) > + { > + std::_Construct(&__u._M_rest); > + std::_Construct(&__u._M_rest._M_rest); > + return &__u._M_rest._M_rest._M_first; > + } > + else > + { > + std::_Construct(&__u._M_rest); > + std::_Construct(&__u._M_rest._M_rest); > + std::_Construct(&__u._M_rest._M_rest._M_rest); > + return __variant::__construct_n<_Np - > 3>(__u._M_rest._M_rest._M_rest); > + } > + } > + > template<typename... _Types> > struct _Traits > { > @@ -536,8 +563,9 @@ namespace __variant > __emplace(_Variant_storage<_Triv, _Types...>& __v, _Args&&... __args) > { > __v._M_reset(); > - auto* __addr = std::__addressof(__variant::__get_n<_Np>(__v._M_u)); > - std::_Construct(__addr, std::forward<_Args>(__args)...); > + auto* __addr = __variant::__construct_n<_Np>(__v._M_u); > + std::_Construct(__addr, in_place_index<0>, > + std::forward<_Args>(__args)...); > // Construction didn't throw, so can set the new index now: > __v._M_index = _Np; > } > -- > 2.41.0 > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libstdc++: Ensure active union member is correctly set 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 1 sibling, 1 reply; 11+ messages in thread From: Jonathan Wakely @ 2023-09-27 14:13 UTC (permalink / raw) To: Nathaniel Shead; +Cc: Jonathan Wakely, Jason Merrill, libstdc++, gcc-patches On Sat, 23 Sept 2023 at 08:30, Nathaniel Shead via Libstdc++ <libstdc++@gcc.gnu.org> wrote: > > On Sat, Sep 23, 2023 at 07:40:48AM +0100, Jonathan Wakely wrote: > > On Sat, 23 Sept 2023, 01:39 Nathaniel Shead via Libstdc++, < > > libstdc++@gcc.gnu.org> wrote: > > > > > Now that bootstrap has finished, I have gotten regressions in the > > > following libstdc++ tests: > > > > > > Running libstdc++:libstdc++-dg/conformance.exp ... > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++23 (test for excess > > > errors) > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++26 (test for excess > > > errors) > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++20 (test for excess errors) > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++26 (test for excess errors) > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++20 (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++26 (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++20 (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++26 (test > > > for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > -std=gnu++20 (test for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > -std=gnu++26 (test for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++20 > > > (test for excess errors) > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++26 > > > (test for excess errors) > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++23 (test for excess > > > errors) > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++23 compilation > > > failed to produce executable > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++26 (test for excess > > > errors) > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++26 compilation > > > failed to produce executable > > > > > > On investigation though it looks like the issue might be with libstdc++ > > > rather than the patch itself; running the failing tests using clang with > > > libstdc++ also produces similar errors, and my reading of the code > > > suggests that this is correct. > > > > > > What's the way forward here? Should I look at creating a patch to fix > > > the libstdc++ issues before resubmitting this patch for the C++ > > > frontend? Or should I submit a version of this patch without the > > > `std::construct_at` changes and wait till libstdc++ gets fixed for that? > > > > > > > I think we should fix libstdc++. There are probably only a few places that > > need a fix, which cause all those failures. > > > > I can help with those fixes. I'll look into it after the weekend. > > > > Thanks. I did end up getting a chance to look at it earlier today, and > with the following patch I had no regressions when applying the frontend > changes. Bootstrapped and regtested on x86_64-pc-linux-gnu. > > -- >8 -- > > This patch ensures that the union members for std::string and > std::variant are always properly set when a change occurs. > > libstdc++-v3/ChangeLog: > > * include/bits/basic_string.h: (basic_string(basic_string&&)): > Activate _M_local_buf when needed. > (basic_string(basic_string&&, const _Alloc&)): Likewise. > * include/bits/basic_string.tcc: (basic_string::swap): Likewise. > * include/std/variant: (__detail::__variant::__construct_n): New. > (__detail::_variant::__emplace): Use __construct_n. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > libstdc++-v3/include/bits/basic_string.h | 7 +++-- > libstdc++-v3/include/bits/basic_string.tcc | 8 +++--- > libstdc++-v3/include/std/variant | 32 ++++++++++++++++++++-- > 3 files changed, 38 insertions(+), 9 deletions(-) > > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h > index 09fd62afa66..7c342879827 100644 > --- a/libstdc++-v3/include/bits/basic_string.h > +++ b/libstdc++-v3/include/bits/basic_string.h > @@ -678,7 +678,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > { > if (__str._M_is_local()) > { > - traits_type::copy(_M_local_buf, __str._M_local_buf, > + traits_type::copy(_M_use_local_data(), __str._M_local_buf, > __str.length() + 1); > } > else > @@ -691,7 +691,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > // basic_stringbuf relies on writing into unallocated capacity so > // we mess up the contents if we put a '\0' in the string. > _M_length(__str.length()); > - __str._M_data(__str._M_local_data()); > + __str._M_data(__str._M_use_local_data()); > __str._M_set_length(0); > } > > @@ -717,6 +717,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > { > if (__str._M_is_local()) > { > + _M_use_local_data(); Lets add a cast to void to make it clear we're intentionally discarding the return value here, and only calling it for its constexpr "side effects": (void) _M_use_local_data(); > traits_type::copy(_M_local_buf, __str._M_local_buf, > __str.length() + 1); > _M_length(__str.length()); > @@ -728,7 +729,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > _M_data(__str._M_data()); > _M_length(__str.length()); > _M_capacity(__str._M_allocated_capacity); > - __str._M_data(__str._M_local_buf); > + __str._M_data(__str._M_use_local_data()); > __str._M_set_length(0); > } > else > diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc > index 104a517f794..ee6e57da555 100644 > --- a/libstdc++-v3/include/bits/basic_string.tcc > +++ b/libstdc++-v3/include/bits/basic_string.tcc > @@ -79,7 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > else if (__s.length()) > { > - traits_type::copy(_M_local_buf, __s._M_local_buf, > + traits_type::copy(_M_use_local_data(), __s._M_local_buf, I think we should call _M_use_local_data() before calling traits_type::copy, as you did for basic_string(basic_string&& __str, const _Alloc& __a) above. The problem is that _M_use_local_data() returns the allocator's pointer type, but traits_type::copy expects char_type*, and those might not be the same type. So: @@ -79,6 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } else if (__s.length()) { + (void) _M_use_local_data(); traits_type::copy(_M_local_buf, __s._M_local_buf, __s.length() + 1); _M_length(__s.length()); > __s.length() + 1); > _M_length(__s.length()); > __s._M_set_length(0); > @@ -87,7 +87,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > else if (length()) > { > - traits_type::copy(__s._M_local_buf, _M_local_buf, > + traits_type::copy(__s._M_use_local_data(), _M_local_buf, Same here, for __s._M_use_local_data() > length() + 1); > __s._M_length(length()); > _M_set_length(0); > @@ -97,7 +97,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > else > { > const size_type __tmp_capacity = __s._M_allocated_capacity; > - traits_type::copy(__s._M_local_buf, _M_local_buf, > + traits_type::copy(__s._M_use_local_data(), _M_local_buf, And again. > length() + 1); > _M_data(__s._M_data()); > __s._M_data(__s._M_local_buf); > @@ -108,7 +108,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > const size_type __tmp_capacity = _M_allocated_capacity; > if (__s._M_is_local()) > { > - traits_type::copy(_M_local_buf, __s._M_local_buf, > + traits_type::copy(_M_use_local_data(), __s._M_local_buf, And again. > __s.length() + 1); > __s._M_data(_M_data()); > _M_data(_M_local_buf); > diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant > index c0e41740dcf..7f24e760bb1 100644 > --- a/libstdc++-v3/include/std/variant > +++ b/libstdc++-v3/include/std/variant > @@ -320,6 +320,33 @@ namespace __variant > __get(_Variant&& __v) noexcept > { return __variant::__get_n<_Np>(std::forward<_Variant>(__v)._M_u); } > > + // Gets the _Uninitialized to construct into for __u. > + template<size_t _Np, typename _Union> > + constexpr decltype(auto) > + __construct_n(_Union& __u) noexcept > + { > + if constexpr (_Np == 0) > + return &__u._M_first; > + else if constexpr (_Np == 1) > + { > + std::_Construct(&__u._M_rest); > + return &__u._M_rest._M_first; > + } > + else if constexpr (_Np == 2) > + { > + std::_Construct(&__u._M_rest); > + std::_Construct(&__u._M_rest._M_rest); > + return &__u._M_rest._M_rest._M_first; > + } > + else > + { > + std::_Construct(&__u._M_rest); > + std::_Construct(&__u._M_rest._M_rest); > + std::_Construct(&__u._M_rest._M_rest._M_rest); > + return __variant::__construct_n<_Np - 3>(__u._M_rest._M_rest._M_rest); > + } > + } This is nice, thanks for optimizing it to reduce the recursion depth. > + > template<typename... _Types> > struct _Traits > { > @@ -536,8 +563,9 @@ namespace __variant > __emplace(_Variant_storage<_Triv, _Types...>& __v, _Args&&... __args) > { > __v._M_reset(); > - auto* __addr = std::__addressof(__variant::__get_n<_Np>(__v._M_u)); > - std::_Construct(__addr, std::forward<_Args>(__args)...); > + auto* __addr = __variant::__construct_n<_Np>(__v._M_u); > + std::_Construct(__addr, in_place_index<0>, > + std::forward<_Args>(__args)...); > // Construction didn't throw, so can set the new index now: > __v._M_index = _Np; > } > -- > 2.41.0 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libstdc++: Ensure active union member is correctly set 2023-09-27 14:13 ` Jonathan Wakely @ 2023-09-28 23:25 ` Nathaniel Shead 2023-09-29 9:32 ` Jonathan Wakely 0 siblings, 1 reply; 11+ messages in thread From: Nathaniel Shead @ 2023-09-28 23:25 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Jonathan Wakely, Jason Merrill, libstdc++, gcc-patches On Wed, Sep 27, 2023 at 03:13:35PM +0100, Jonathan Wakely wrote: > On Sat, 23 Sept 2023 at 08:30, Nathaniel Shead via Libstdc++ > <libstdc++@gcc.gnu.org> wrote: > > > > On Sat, Sep 23, 2023 at 07:40:48AM +0100, Jonathan Wakely wrote: > > > On Sat, 23 Sept 2023, 01:39 Nathaniel Shead via Libstdc++, < > > > libstdc++@gcc.gnu.org> wrote: > > > > > > > Now that bootstrap has finished, I have gotten regressions in the > > > > following libstdc++ tests: > > > > > > > > Running libstdc++:libstdc++-dg/conformance.exp ... > > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++23 (test for excess > > > > errors) > > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++26 (test for excess > > > > errors) > > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++20 (test for excess errors) > > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++26 (test for excess errors) > > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++20 (test > > > > for excess errors) > > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++26 (test > > > > for excess errors) > > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++20 (test > > > > for excess errors) > > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++26 (test > > > > for excess errors) > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > > -std=gnu++20 (test for excess errors) > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > > -std=gnu++26 (test for excess errors) > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++20 > > > > (test for excess errors) > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++26 > > > > (test for excess errors) > > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++23 (test for excess > > > > errors) > > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++23 compilation > > > > failed to produce executable > > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++26 (test for excess > > > > errors) > > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++26 compilation > > > > failed to produce executable > > > > > > > > On investigation though it looks like the issue might be with libstdc++ > > > > rather than the patch itself; running the failing tests using clang with > > > > libstdc++ also produces similar errors, and my reading of the code > > > > suggests that this is correct. > > > > > > > > What's the way forward here? Should I look at creating a patch to fix > > > > the libstdc++ issues before resubmitting this patch for the C++ > > > > frontend? Or should I submit a version of this patch without the > > > > `std::construct_at` changes and wait till libstdc++ gets fixed for that? > > > > > > > > > > I think we should fix libstdc++. There are probably only a few places that > > > need a fix, which cause all those failures. > > > > > > I can help with those fixes. I'll look into it after the weekend. > > > > > > > Thanks. I did end up getting a chance to look at it earlier today, and > > with the following patch I had no regressions when applying the frontend > > changes. Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > -- >8 -- > > > > This patch ensures that the union members for std::string and > > std::variant are always properly set when a change occurs. > > > > libstdc++-v3/ChangeLog: > > > > * include/bits/basic_string.h: (basic_string(basic_string&&)): > > Activate _M_local_buf when needed. > > (basic_string(basic_string&&, const _Alloc&)): Likewise. > > * include/bits/basic_string.tcc: (basic_string::swap): Likewise. > > * include/std/variant: (__detail::__variant::__construct_n): New. > > (__detail::_variant::__emplace): Use __construct_n. > > > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > > --- > > libstdc++-v3/include/bits/basic_string.h | 7 +++-- > > libstdc++-v3/include/bits/basic_string.tcc | 8 +++--- > > libstdc++-v3/include/std/variant | 32 ++++++++++++++++++++-- > > 3 files changed, 38 insertions(+), 9 deletions(-) > > > > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h > > index 09fd62afa66..7c342879827 100644 > > --- a/libstdc++-v3/include/bits/basic_string.h > > +++ b/libstdc++-v3/include/bits/basic_string.h > > @@ -678,7 +678,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > { > > if (__str._M_is_local()) > > { > > - traits_type::copy(_M_local_buf, __str._M_local_buf, > > + traits_type::copy(_M_use_local_data(), __str._M_local_buf, > > __str.length() + 1); > > } > > else > > @@ -691,7 +691,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > // basic_stringbuf relies on writing into unallocated capacity so > > // we mess up the contents if we put a '\0' in the string. > > _M_length(__str.length()); > > - __str._M_data(__str._M_local_data()); > > + __str._M_data(__str._M_use_local_data()); > > __str._M_set_length(0); > > } > > > > @@ -717,6 +717,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > { > > if (__str._M_is_local()) > > { > > + _M_use_local_data(); > > Lets add a cast to void to make it clear we're intentionally > discarding the return value here, and only calling it for its > constexpr "side effects": > > (void) _M_use_local_data(); > > > > traits_type::copy(_M_local_buf, __str._M_local_buf, > > __str.length() + 1); > > _M_length(__str.length()); > > @@ -728,7 +729,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > _M_data(__str._M_data()); > > _M_length(__str.length()); > > _M_capacity(__str._M_allocated_capacity); > > - __str._M_data(__str._M_local_buf); > > + __str._M_data(__str._M_use_local_data()); > > __str._M_set_length(0); > > } > > else > > diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc > > index 104a517f794..ee6e57da555 100644 > > --- a/libstdc++-v3/include/bits/basic_string.tcc > > +++ b/libstdc++-v3/include/bits/basic_string.tcc > > @@ -79,7 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > } > > else if (__s.length()) > > { > > - traits_type::copy(_M_local_buf, __s._M_local_buf, > > + traits_type::copy(_M_use_local_data(), __s._M_local_buf, > > I think we should call _M_use_local_data() before calling > traits_type::copy, as you did for > basic_string(basic_string&& __str, const _Alloc& __a) > above. > > The problem is that _M_use_local_data() returns the allocator's > pointer type, but traits_type::copy expects char_type*, and those > might not be the same type. > So: > > @@ -79,6 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > else if (__s.length()) > { > + (void) _M_use_local_data(); > traits_type::copy(_M_local_buf, __s._M_local_buf, > __s.length() + 1); > _M_length(__s.length()); > > > > > __s.length() + 1); > > _M_length(__s.length()); > > __s._M_set_length(0); > > @@ -87,7 +87,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > } > > else if (length()) > > { > > - traits_type::copy(__s._M_local_buf, _M_local_buf, > > + traits_type::copy(__s._M_use_local_data(), _M_local_buf, > > Same here, for __s._M_use_local_data() > > > length() + 1); > > __s._M_length(length()); > > _M_set_length(0); > > @@ -97,7 +97,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > else > > { > > const size_type __tmp_capacity = __s._M_allocated_capacity; > > - traits_type::copy(__s._M_local_buf, _M_local_buf, > > + traits_type::copy(__s._M_use_local_data(), _M_local_buf, > > And again. > > > length() + 1); > > _M_data(__s._M_data()); > > __s._M_data(__s._M_local_buf); > > @@ -108,7 +108,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > const size_type __tmp_capacity = _M_allocated_capacity; > > if (__s._M_is_local()) > > { > > - traits_type::copy(_M_local_buf, __s._M_local_buf, > > + traits_type::copy(_M_use_local_data(), __s._M_local_buf, > > And again. > > > __s.length() + 1); > > __s._M_data(_M_data()); > > _M_data(_M_local_buf); > > diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant > > index c0e41740dcf..7f24e760bb1 100644 > > --- a/libstdc++-v3/include/std/variant > > +++ b/libstdc++-v3/include/std/variant > > @@ -320,6 +320,33 @@ namespace __variant > > __get(_Variant&& __v) noexcept > > { return __variant::__get_n<_Np>(std::forward<_Variant>(__v)._M_u); } > > > > + // Gets the _Uninitialized to construct into for __u. > > + template<size_t _Np, typename _Union> > > + constexpr decltype(auto) > > + __construct_n(_Union& __u) noexcept > > + { > > + if constexpr (_Np == 0) > > + return &__u._M_first; > > + else if constexpr (_Np == 1) > > + { > > + std::_Construct(&__u._M_rest); > > + return &__u._M_rest._M_first; > > + } > > + else if constexpr (_Np == 2) > > + { > > + std::_Construct(&__u._M_rest); > > + std::_Construct(&__u._M_rest._M_rest); > > + return &__u._M_rest._M_rest._M_first; > > + } > > + else > > + { > > + std::_Construct(&__u._M_rest); > > + std::_Construct(&__u._M_rest._M_rest); > > + std::_Construct(&__u._M_rest._M_rest._M_rest); > > + return __variant::__construct_n<_Np - 3>(__u._M_rest._M_rest._M_rest); > > + } > > + } > > This is nice, thanks for optimizing it to reduce the recursion depth. > > > + > > template<typename... _Types> > > struct _Traits > > { > > @@ -536,8 +563,9 @@ namespace __variant > > __emplace(_Variant_storage<_Triv, _Types...>& __v, _Args&&... __args) > > { > > __v._M_reset(); > > - auto* __addr = std::__addressof(__variant::__get_n<_Np>(__v._M_u)); > > - std::_Construct(__addr, std::forward<_Args>(__args)...); > > + auto* __addr = __variant::__construct_n<_Np>(__v._M_u); > > + std::_Construct(__addr, in_place_index<0>, > > + std::forward<_Args>(__args)...); > > // Construction didn't throw, so can set the new index now: > > __v._M_index = _Np; > > } > > -- > > 2.41.0 > > > Thanks for the comments, here's an updated version of the patch. Bootstrapped and regtested on x86_64-pc-linux-gnu. I'll note that there are some existing calls to `_M_use_local_data()` already used only for their side effects without a cast to void, e.g. /** * @brief Default constructor creates an empty string. */ _GLIBCXX20_CONSTEXPR basic_string() _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) : _M_dataplus(_M_local_data()) { _M_use_local_data(); _M_set_length(0); } I haven't updated these, but should this be changed for consistency? -- >8 -- This patch ensures that the union members for std::string and std::variant are always properly set when a change occurs. libstdc++-v3/ChangeLog: * include/bits/basic_string.h: (basic_string(basic_string&&)): Activate _M_local_buf when needed. (basic_string(basic_string&&, const _Alloc&)): Likewise. * include/bits/basic_string.tcc: (basic_string::swap): Likewise. * include/std/variant: (__detail::__variant::__construct_n): New. (__detail::__variant::__emplace): Use __construct_n. Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> --- libstdc++-v3/include/bits/basic_string.h | 6 ++-- libstdc++-v3/include/bits/basic_string.tcc | 4 +++ libstdc++-v3/include/std/variant | 32 ++++++++++++++++++++-- 3 files changed, 38 insertions(+), 4 deletions(-) diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index 09fd62afa66..4f94cd967cf 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -678,6 +678,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { if (__str._M_is_local()) { + (void)_M_use_local_data(); traits_type::copy(_M_local_buf, __str._M_local_buf, __str.length() + 1); } @@ -691,7 +692,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 // basic_stringbuf relies on writing into unallocated capacity so // we mess up the contents if we put a '\0' in the string. _M_length(__str.length()); - __str._M_data(__str._M_local_data()); + __str._M_data(__str._M_use_local_data()); __str._M_set_length(0); } @@ -717,6 +718,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { if (__str._M_is_local()) { + (void)_M_use_local_data(); traits_type::copy(_M_local_buf, __str._M_local_buf, __str.length() + 1); _M_length(__str.length()); @@ -728,7 +730,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 _M_data(__str._M_data()); _M_length(__str.length()); _M_capacity(__str._M_allocated_capacity); - __str._M_data(__str._M_local_buf); + __str._M_data(__str._M_use_local_data()); __str._M_set_length(0); } else diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc index 104a517f794..4bc98f2aea7 100644 --- a/libstdc++-v3/include/bits/basic_string.tcc +++ b/libstdc++-v3/include/bits/basic_string.tcc @@ -79,6 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } else if (__s.length()) { + (void)_M_use_local_data(); traits_type::copy(_M_local_buf, __s._M_local_buf, __s.length() + 1); _M_length(__s.length()); @@ -87,6 +88,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } else if (length()) { + (void)__s._M_use_local_data(); traits_type::copy(__s._M_local_buf, _M_local_buf, length() + 1); __s._M_length(length()); @@ -97,6 +99,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION else { const size_type __tmp_capacity = __s._M_allocated_capacity; + (void)__s._M_use_local_data(); traits_type::copy(__s._M_local_buf, _M_local_buf, length() + 1); _M_data(__s._M_data()); @@ -108,6 +111,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const size_type __tmp_capacity = _M_allocated_capacity; if (__s._M_is_local()) { + (void)_M_use_local_data(); traits_type::copy(_M_local_buf, __s._M_local_buf, __s.length() + 1); __s._M_data(_M_data()); diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant index c0e41740dcf..7f24e760bb1 100644 --- a/libstdc++-v3/include/std/variant +++ b/libstdc++-v3/include/std/variant @@ -320,6 +320,33 @@ namespace __variant __get(_Variant&& __v) noexcept { return __variant::__get_n<_Np>(std::forward<_Variant>(__v)._M_u); } + // Gets the _Uninitialized to construct into for __u. + template<size_t _Np, typename _Union> + constexpr decltype(auto) + __construct_n(_Union& __u) noexcept + { + if constexpr (_Np == 0) + return &__u._M_first; + else if constexpr (_Np == 1) + { + std::_Construct(&__u._M_rest); + return &__u._M_rest._M_first; + } + else if constexpr (_Np == 2) + { + std::_Construct(&__u._M_rest); + std::_Construct(&__u._M_rest._M_rest); + return &__u._M_rest._M_rest._M_first; + } + else + { + std::_Construct(&__u._M_rest); + std::_Construct(&__u._M_rest._M_rest); + std::_Construct(&__u._M_rest._M_rest._M_rest); + return __variant::__construct_n<_Np - 3>(__u._M_rest._M_rest._M_rest); + } + } + template<typename... _Types> struct _Traits { @@ -536,8 +563,9 @@ namespace __variant __emplace(_Variant_storage<_Triv, _Types...>& __v, _Args&&... __args) { __v._M_reset(); - auto* __addr = std::__addressof(__variant::__get_n<_Np>(__v._M_u)); - std::_Construct(__addr, std::forward<_Args>(__args)...); + auto* __addr = __variant::__construct_n<_Np>(__v._M_u); + std::_Construct(__addr, in_place_index<0>, + std::forward<_Args>(__args)...); // Construction didn't throw, so can set the new index now: __v._M_index = _Np; } -- 2.41.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libstdc++: Ensure active union member is correctly set 2023-09-28 23:25 ` Nathaniel Shead @ 2023-09-29 9:32 ` Jonathan Wakely 2023-09-29 15:06 ` Jonathan Wakely 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Wakely @ 2023-09-29 9:32 UTC (permalink / raw) To: Nathaniel Shead; +Cc: Jonathan Wakely, Jason Merrill, libstdc++, gcc-patches On Fri, 29 Sept 2023 at 00:25, Nathaniel Shead <nathanieloshead@gmail.com> wrote: > > On Wed, Sep 27, 2023 at 03:13:35PM +0100, Jonathan Wakely wrote: > > On Sat, 23 Sept 2023 at 08:30, Nathaniel Shead via Libstdc++ > > <libstdc++@gcc.gnu.org> wrote: > > > > > > On Sat, Sep 23, 2023 at 07:40:48AM +0100, Jonathan Wakely wrote: > > > > On Sat, 23 Sept 2023, 01:39 Nathaniel Shead via Libstdc++, < > > > > libstdc++@gcc.gnu.org> wrote: > > > > > > > > > Now that bootstrap has finished, I have gotten regressions in the > > > > > following libstdc++ tests: > > > > > > > > > > Running libstdc++:libstdc++-dg/conformance.exp ... > > > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++23 (test for excess > > > > > errors) > > > > > FAIL: 20_util/bitset/access/constexpr.cc -std=gnu++26 (test for excess > > > > > errors) > > > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++20 (test for excess errors) > > > > > FAIL: 20_util/variant/constexpr.cc -std=gnu++26 (test for excess errors) > > > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++20 (test > > > > > for excess errors) > > > > > FAIL: 21_strings/basic_string/cons/char/constexpr.cc -std=gnu++26 (test > > > > > for excess errors) > > > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++20 (test > > > > > for excess errors) > > > > > FAIL: 21_strings/basic_string/cons/wchar_t/constexpr.cc -std=gnu++26 (test > > > > > for excess errors) > > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > > > -std=gnu++20 (test for excess errors) > > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr-wchar_t.cc > > > > > -std=gnu++26 (test for excess errors) > > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++20 > > > > > (test for excess errors) > > > > > FAIL: 21_strings/basic_string/modifiers/swap/constexpr.cc -std=gnu++26 > > > > > (test for excess errors) > > > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++23 (test for excess > > > > > errors) > > > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++23 compilation > > > > > failed to produce executable > > > > > FAIL: std/ranges/adaptors/join_with/1.cc -std=gnu++26 (test for excess > > > > > errors) > > > > > UNRESOLVED: std/ranges/adaptors/join_with/1.cc -std=gnu++26 compilation > > > > > failed to produce executable > > > > > > > > > > On investigation though it looks like the issue might be with libstdc++ > > > > > rather than the patch itself; running the failing tests using clang with > > > > > libstdc++ also produces similar errors, and my reading of the code > > > > > suggests that this is correct. > > > > > > > > > > What's the way forward here? Should I look at creating a patch to fix > > > > > the libstdc++ issues before resubmitting this patch for the C++ > > > > > frontend? Or should I submit a version of this patch without the > > > > > `std::construct_at` changes and wait till libstdc++ gets fixed for that? > > > > > > > > > > > > > I think we should fix libstdc++. There are probably only a few places that > > > > need a fix, which cause all those failures. > > > > > > > > I can help with those fixes. I'll look into it after the weekend. > > > > > > > > > > Thanks. I did end up getting a chance to look at it earlier today, and > > > with the following patch I had no regressions when applying the frontend > > > changes. Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > > > -- >8 -- > > > > > > This patch ensures that the union members for std::string and > > > std::variant are always properly set when a change occurs. > > > > > > libstdc++-v3/ChangeLog: > > > > > > * include/bits/basic_string.h: (basic_string(basic_string&&)): > > > Activate _M_local_buf when needed. > > > (basic_string(basic_string&&, const _Alloc&)): Likewise. > > > * include/bits/basic_string.tcc: (basic_string::swap): Likewise. > > > * include/std/variant: (__detail::__variant::__construct_n): New. > > > (__detail::_variant::__emplace): Use __construct_n. > > > > > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > > > --- > > > libstdc++-v3/include/bits/basic_string.h | 7 +++-- > > > libstdc++-v3/include/bits/basic_string.tcc | 8 +++--- > > > libstdc++-v3/include/std/variant | 32 ++++++++++++++++++++-- > > > 3 files changed, 38 insertions(+), 9 deletions(-) > > > > > > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h > > > index 09fd62afa66..7c342879827 100644 > > > --- a/libstdc++-v3/include/bits/basic_string.h > > > +++ b/libstdc++-v3/include/bits/basic_string.h > > > @@ -678,7 +678,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > > { > > > if (__str._M_is_local()) > > > { > > > - traits_type::copy(_M_local_buf, __str._M_local_buf, > > > + traits_type::copy(_M_use_local_data(), __str._M_local_buf, > > > __str.length() + 1); > > > } > > > else > > > @@ -691,7 +691,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > > // basic_stringbuf relies on writing into unallocated capacity so > > > // we mess up the contents if we put a '\0' in the string. > > > _M_length(__str.length()); > > > - __str._M_data(__str._M_local_data()); > > > + __str._M_data(__str._M_use_local_data()); > > > __str._M_set_length(0); > > > } > > > > > > @@ -717,6 +717,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > > { > > > if (__str._M_is_local()) > > > { > > > + _M_use_local_data(); > > > > Lets add a cast to void to make it clear we're intentionally > > discarding the return value here, and only calling it for its > > constexpr "side effects": > > > > (void) _M_use_local_data(); > > > > > > > traits_type::copy(_M_local_buf, __str._M_local_buf, > > > __str.length() + 1); > > > _M_length(__str.length()); > > > @@ -728,7 +729,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > > _M_data(__str._M_data()); > > > _M_length(__str.length()); > > > _M_capacity(__str._M_allocated_capacity); > > > - __str._M_data(__str._M_local_buf); > > > + __str._M_data(__str._M_use_local_data()); > > > __str._M_set_length(0); > > > } > > > else > > > diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc > > > index 104a517f794..ee6e57da555 100644 > > > --- a/libstdc++-v3/include/bits/basic_string.tcc > > > +++ b/libstdc++-v3/include/bits/basic_string.tcc > > > @@ -79,7 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > } > > > else if (__s.length()) > > > { > > > - traits_type::copy(_M_local_buf, __s._M_local_buf, > > > + traits_type::copy(_M_use_local_data(), __s._M_local_buf, > > > > I think we should call _M_use_local_data() before calling > > traits_type::copy, as you did for > > basic_string(basic_string&& __str, const _Alloc& __a) > > above. > > > > The problem is that _M_use_local_data() returns the allocator's > > pointer type, but traits_type::copy expects char_type*, and those > > might not be the same type. > > So: > > > > @@ -79,6 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > } > > else if (__s.length()) > > { > > + (void) _M_use_local_data(); > > traits_type::copy(_M_local_buf, __s._M_local_buf, > > __s.length() + 1); > > _M_length(__s.length()); > > > > > > > > > __s.length() + 1); > > > _M_length(__s.length()); > > > __s._M_set_length(0); > > > @@ -87,7 +87,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > } > > > else if (length()) > > > { > > > - traits_type::copy(__s._M_local_buf, _M_local_buf, > > > + traits_type::copy(__s._M_use_local_data(), _M_local_buf, > > > > Same here, for __s._M_use_local_data() > > > > > length() + 1); > > > __s._M_length(length()); > > > _M_set_length(0); > > > @@ -97,7 +97,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > else > > > { > > > const size_type __tmp_capacity = __s._M_allocated_capacity; > > > - traits_type::copy(__s._M_local_buf, _M_local_buf, > > > + traits_type::copy(__s._M_use_local_data(), _M_local_buf, > > > > And again. > > > > > length() + 1); > > > _M_data(__s._M_data()); > > > __s._M_data(__s._M_local_buf); > > > @@ -108,7 +108,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > const size_type __tmp_capacity = _M_allocated_capacity; > > > if (__s._M_is_local()) > > > { > > > - traits_type::copy(_M_local_buf, __s._M_local_buf, > > > + traits_type::copy(_M_use_local_data(), __s._M_local_buf, > > > > And again. > > > > > __s.length() + 1); > > > __s._M_data(_M_data()); > > > _M_data(_M_local_buf); > > > diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant > > > index c0e41740dcf..7f24e760bb1 100644 > > > --- a/libstdc++-v3/include/std/variant > > > +++ b/libstdc++-v3/include/std/variant > > > @@ -320,6 +320,33 @@ namespace __variant > > > __get(_Variant&& __v) noexcept > > > { return __variant::__get_n<_Np>(std::forward<_Variant>(__v)._M_u); } > > > > > > + // Gets the _Uninitialized to construct into for __u. > > > + template<size_t _Np, typename _Union> > > > + constexpr decltype(auto) > > > + __construct_n(_Union& __u) noexcept > > > + { > > > + if constexpr (_Np == 0) > > > + return &__u._M_first; > > > + else if constexpr (_Np == 1) > > > + { > > > + std::_Construct(&__u._M_rest); > > > + return &__u._M_rest._M_first; > > > + } > > > + else if constexpr (_Np == 2) > > > + { > > > + std::_Construct(&__u._M_rest); > > > + std::_Construct(&__u._M_rest._M_rest); > > > + return &__u._M_rest._M_rest._M_first; > > > + } > > > + else > > > + { > > > + std::_Construct(&__u._M_rest); > > > + std::_Construct(&__u._M_rest._M_rest); > > > + std::_Construct(&__u._M_rest._M_rest._M_rest); > > > + return __variant::__construct_n<_Np - 3>(__u._M_rest._M_rest._M_rest); > > > + } > > > + } > > > > This is nice, thanks for optimizing it to reduce the recursion depth. > > > > > + > > > template<typename... _Types> > > > struct _Traits > > > { > > > @@ -536,8 +563,9 @@ namespace __variant > > > __emplace(_Variant_storage<_Triv, _Types...>& __v, _Args&&... __args) > > > { > > > __v._M_reset(); > > > - auto* __addr = std::__addressof(__variant::__get_n<_Np>(__v._M_u)); > > > - std::_Construct(__addr, std::forward<_Args>(__args)...); > > > + auto* __addr = __variant::__construct_n<_Np>(__v._M_u); > > > + std::_Construct(__addr, in_place_index<0>, > > > + std::forward<_Args>(__args)...); > > > // Construction didn't throw, so can set the new index now: > > > __v._M_index = _Np; > > > } > > > -- > > > 2.41.0 > > > > > > > Thanks for the comments, here's an updated version of the patch. > Bootstrapped and regtested on x86_64-pc-linux-gnu. Great, I'll get this committed today - thanks! > > I'll note that there are some existing calls to `_M_use_local_data()` > already used only for their side effects without a cast to void, e.g. > > /** > * @brief Default constructor creates an empty string. > */ > _GLIBCXX20_CONSTEXPR > basic_string() > _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) > : _M_dataplus(_M_local_data()) > { > _M_use_local_data(); > _M_set_length(0); > } > > I haven't updated these, but should this be changed for consistency? Yes, good idea. I can do that. Thanks again for fixing these. I think this might fix some bug reports about clang rejecting our std::string in constant expressions, so I'll check those. > > -- >8 -- > > This patch ensures that the union members for std::string and > std::variant are always properly set when a change occurs. > > libstdc++-v3/ChangeLog: > > * include/bits/basic_string.h: (basic_string(basic_string&&)): > Activate _M_local_buf when needed. > (basic_string(basic_string&&, const _Alloc&)): Likewise. > * include/bits/basic_string.tcc: (basic_string::swap): Likewise. > * include/std/variant: (__detail::__variant::__construct_n): New. > (__detail::__variant::__emplace): Use __construct_n. > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com> > --- > libstdc++-v3/include/bits/basic_string.h | 6 ++-- > libstdc++-v3/include/bits/basic_string.tcc | 4 +++ > libstdc++-v3/include/std/variant | 32 ++++++++++++++++++++-- > 3 files changed, 38 insertions(+), 4 deletions(-) > > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h > index 09fd62afa66..4f94cd967cf 100644 > --- a/libstdc++-v3/include/bits/basic_string.h > +++ b/libstdc++-v3/include/bits/basic_string.h > @@ -678,6 +678,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > { > if (__str._M_is_local()) > { > + (void)_M_use_local_data(); > traits_type::copy(_M_local_buf, __str._M_local_buf, > __str.length() + 1); > } > @@ -691,7 +692,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > // basic_stringbuf relies on writing into unallocated capacity so > // we mess up the contents if we put a '\0' in the string. > _M_length(__str.length()); > - __str._M_data(__str._M_local_data()); > + __str._M_data(__str._M_use_local_data()); > __str._M_set_length(0); > } > > @@ -717,6 +718,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > { > if (__str._M_is_local()) > { > + (void)_M_use_local_data(); > traits_type::copy(_M_local_buf, __str._M_local_buf, > __str.length() + 1); > _M_length(__str.length()); > @@ -728,7 +730,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > _M_data(__str._M_data()); > _M_length(__str.length()); > _M_capacity(__str._M_allocated_capacity); > - __str._M_data(__str._M_local_buf); > + __str._M_data(__str._M_use_local_data()); > __str._M_set_length(0); > } > else > diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc > index 104a517f794..4bc98f2aea7 100644 > --- a/libstdc++-v3/include/bits/basic_string.tcc > +++ b/libstdc++-v3/include/bits/basic_string.tcc > @@ -79,6 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > else if (__s.length()) > { > + (void)_M_use_local_data(); > traits_type::copy(_M_local_buf, __s._M_local_buf, > __s.length() + 1); > _M_length(__s.length()); > @@ -87,6 +88,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > else if (length()) > { > + (void)__s._M_use_local_data(); > traits_type::copy(__s._M_local_buf, _M_local_buf, > length() + 1); > __s._M_length(length()); > @@ -97,6 +99,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > else > { > const size_type __tmp_capacity = __s._M_allocated_capacity; > + (void)__s._M_use_local_data(); > traits_type::copy(__s._M_local_buf, _M_local_buf, > length() + 1); > _M_data(__s._M_data()); > @@ -108,6 +111,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > const size_type __tmp_capacity = _M_allocated_capacity; > if (__s._M_is_local()) > { > + (void)_M_use_local_data(); > traits_type::copy(_M_local_buf, __s._M_local_buf, > __s.length() + 1); > __s._M_data(_M_data()); > diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant > index c0e41740dcf..7f24e760bb1 100644 > --- a/libstdc++-v3/include/std/variant > +++ b/libstdc++-v3/include/std/variant > @@ -320,6 +320,33 @@ namespace __variant > __get(_Variant&& __v) noexcept > { return __variant::__get_n<_Np>(std::forward<_Variant>(__v)._M_u); } > > + // Gets the _Uninitialized to construct into for __u. > + template<size_t _Np, typename _Union> > + constexpr decltype(auto) > + __construct_n(_Union& __u) noexcept > + { > + if constexpr (_Np == 0) > + return &__u._M_first; > + else if constexpr (_Np == 1) > + { > + std::_Construct(&__u._M_rest); > + return &__u._M_rest._M_first; > + } > + else if constexpr (_Np == 2) > + { > + std::_Construct(&__u._M_rest); > + std::_Construct(&__u._M_rest._M_rest); > + return &__u._M_rest._M_rest._M_first; > + } > + else > + { > + std::_Construct(&__u._M_rest); > + std::_Construct(&__u._M_rest._M_rest); > + std::_Construct(&__u._M_rest._M_rest._M_rest); > + return __variant::__construct_n<_Np - 3>(__u._M_rest._M_rest._M_rest); > + } > + } > + > template<typename... _Types> > struct _Traits > { > @@ -536,8 +563,9 @@ namespace __variant > __emplace(_Variant_storage<_Triv, _Types...>& __v, _Args&&... __args) > { > __v._M_reset(); > - auto* __addr = std::__addressof(__variant::__get_n<_Np>(__v._M_u)); > - std::_Construct(__addr, std::forward<_Args>(__args)...); > + auto* __addr = __variant::__construct_n<_Np>(__v._M_u); > + std::_Construct(__addr, in_place_index<0>, > + std::forward<_Args>(__args)...); > // Construction didn't throw, so can set the new index now: > __v._M_index = _Np; > } > -- > 2.41.0 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libstdc++: Ensure active union member is correctly set 2023-09-29 9:32 ` Jonathan Wakely @ 2023-09-29 15:06 ` Jonathan Wakely 2023-09-29 16:29 ` Nathaniel Shead 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Wakely @ 2023-09-29 15:06 UTC (permalink / raw) To: Nathaniel Shead; +Cc: Jonathan Wakely, Jason Merrill, libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 1688 bytes --] On Fri, 29 Sept 2023 at 10:32, Jonathan Wakely <jwakely@redhat.com> wrote: > > Thanks for the comments, here's an updated version of the patch. > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > Great, I'll get this committed today - thanks! That's done now. > > > > I'll note that there are some existing calls to `_M_use_local_data()` > > already used only for their side effects without a cast to void, e.g. > > > > /** > > * @brief Default constructor creates an empty string. > > */ > > _GLIBCXX20_CONSTEXPR > > basic_string() > > _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) > > : _M_dataplus(_M_local_data()) > > { > > _M_use_local_data(); > > _M_set_length(0); > > } > > > > I haven't updated these, but should this be changed for consistency? > > Yes, good idea. I can do that. I started to do that, and decided it made more sense to split out the constexpr loop from _M_use_local_data() into a separate function, _M_init_local_buf(). Then we can use that for all the places where we don't care about the return value. That avoids the overhead of using pointer_traits::pointer_to when we don't need the return value (which is admittedly only going to be an issue for -O0 code, but I think it's cleaner this way anyway). Please see the attached patch and let me know what you think. > Thanks again for fixing these. I think this might fix some bug reports > about clang rejecting our std::string in constant expressions, so I'll > check those. Your patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110900 (so we should backport it to gcc-13 and gcc-12 too). [-- Attachment #2: patch.txt --] [-- Type: text/plain, Size: 6292 bytes --] commit 2668979d3206ff6c039ac0165aae29377a15666c Author: Jonathan Wakely <jwakely@redhat.com> Date: Fri Sep 29 12:12:22 2023 libstdc++: Split std::basic_string::_M_use_local_data into two functions This splits out the activate-the-union-member-for-constexpr logic from _M_use_local_data, so that it can be used separately in cases that don't need to use std::pointer_traits<pointer>::pointer_to to obtain the return value. This leaves only three uses of _M_use_local_data() which are all the same form: __s._M_data(_M_use_local_data()); __s._M_set_length(0); We could remove _M_use_local_data() and change those three places to use a new _M_reset() function that does: _M_init_local_buf(); _M_data(_M_local_data()); _M_set_length(0); This is left for a future change. libstdc++-v3/ChangeLog: * include/bits/basic_string.h (_M_init_local_buf()): New function. (_M_use_local_data()): Use _M_init_local_buf. (basic_string(), basic_string(const Alloc&)) (basic_string(basic_string&&)) (basic_string(basic_string&&, const Alloc&)): Use _M_init_local_buf instead of _M_use_local_data(). * include/bits/basic_string.tcc (swap(basic_string&)) (_M_construct(InIter, InIter, forward_iterator_tag)) (_M_construct(size_type, CharT), reserve()): Likewise. (_M_construct(InIter, InIter, input_iterator_tag)): Remove call to _M_use_local_data() and initialize the local buffer directly. diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index 4f94cd967cf..18a19b8dcbc 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -353,13 +353,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 // Ensure that _M_local_buf is the active member of the union. __attribute__((__always_inline__)) _GLIBCXX14_CONSTEXPR - pointer - _M_use_local_data() _GLIBCXX_NOEXCEPT + void + _M_init_local_buf() _GLIBCXX_NOEXCEPT { #if __cpp_lib_is_constant_evaluated if (std::is_constant_evaluated()) for (size_type __i = 0; __i <= _S_local_capacity; ++__i) _M_local_buf[__i] = _CharT(); +#endif + } + + __attribute__((__always_inline__)) + _GLIBCXX14_CONSTEXPR + pointer + _M_use_local_data() _GLIBCXX_NOEXCEPT + { +#if __glibcxx_is_constant_evaluated + _M_init_local_buf(); #endif return _M_local_data(); } @@ -522,7 +532,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) : _M_dataplus(_M_local_data()) { - _M_use_local_data(); + _M_init_local_buf(); _M_set_length(0); } @@ -534,7 +544,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 basic_string(const _Alloc& __a) _GLIBCXX_NOEXCEPT : _M_dataplus(_M_local_data(), __a) { - _M_use_local_data(); + _M_init_local_buf(); _M_set_length(0); } @@ -678,7 +688,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { if (__str._M_is_local()) { - (void)_M_use_local_data(); + _M_init_local_buf(); traits_type::copy(_M_local_buf, __str._M_local_buf, __str.length() + 1); } @@ -718,7 +728,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { if (__str._M_is_local()) { - (void)_M_use_local_data(); + _M_init_local_buf(); traits_type::copy(_M_local_buf, __str._M_local_buf, __str.length() + 1); _M_length(__str.length()); diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc index 4bc98f2aea7..052eeb9e846 100644 --- a/libstdc++-v3/include/bits/basic_string.tcc +++ b/libstdc++-v3/include/bits/basic_string.tcc @@ -79,7 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } else if (__s.length()) { - (void)_M_use_local_data(); + _M_init_local_buf(); traits_type::copy(_M_local_buf, __s._M_local_buf, __s.length() + 1); _M_length(__s.length()); @@ -88,7 +88,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } else if (length()) { - (void)__s._M_use_local_data(); + __s._M_init_local_buf(); traits_type::copy(__s._M_local_buf, _M_local_buf, length() + 1); __s._M_length(length()); @@ -99,7 +99,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION else { const size_type __tmp_capacity = __s._M_allocated_capacity; - (void)__s._M_use_local_data(); + __s._M_init_local_buf(); traits_type::copy(__s._M_local_buf, _M_local_buf, length() + 1); _M_data(__s._M_data()); @@ -111,7 +111,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const size_type __tmp_capacity = _M_allocated_capacity; if (__s._M_is_local()) { - (void)_M_use_local_data(); + _M_init_local_buf(); traits_type::copy(_M_local_buf, __s._M_local_buf, __s.length() + 1); __s._M_data(_M_data()); @@ -174,14 +174,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION size_type __len = 0; size_type __capacity = size_type(_S_local_capacity); - pointer __p = _M_use_local_data(); - while (__beg != __end && __len < __capacity) { - __p[__len++] = *__beg; + _M_local_buf[__len++] = *__beg; ++__beg; } +#if __glibcxx_is_constant_evaluated + if (std::is_constant_evaluated()) + for (size_type __i = __len; __i <= __capacity; ++__i) + _M_local_buf[__i] = _CharT(); +#endif + struct _Guard { _GLIBCXX20_CONSTEXPR @@ -230,7 +234,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_capacity(__dnew); } else - _M_use_local_data(); + _M_init_local_buf(); // Check for out_of_range and length_error exceptions. struct _Guard @@ -263,7 +267,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_capacity(__n); } else - _M_use_local_data(); + _M_init_local_buf(); if (__n) this->_S_assign(_M_data(), __n, __c); @@ -372,7 +376,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (__length <= size_type(_S_local_capacity)) { - this->_S_copy(_M_use_local_data(), _M_data(), __length + 1); + _M_init_local_buf(); + this->_S_copy(_M_local_buf, _M_data(), __length + 1); _M_destroy(__capacity); _M_data(_M_local_data()); } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libstdc++: Ensure active union member is correctly set 2023-09-29 15:06 ` Jonathan Wakely @ 2023-09-29 16:29 ` Nathaniel Shead 2023-09-29 16:46 ` Jonathan Wakely 0 siblings, 1 reply; 11+ messages in thread From: Nathaniel Shead @ 2023-09-29 16:29 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Jonathan Wakely, Jason Merrill, libstdc++, gcc-patches On Fri, Sep 29, 2023 at 04:06:33PM +0100, Jonathan Wakely wrote: > On Fri, 29 Sept 2023 at 10:32, Jonathan Wakely <jwakely@redhat.com> wrote: > > > Thanks for the comments, here's an updated version of the patch. > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > Great, I'll get this committed today - thanks! > > That's done now. > Thanks! > > > > > > I'll note that there are some existing calls to `_M_use_local_data()` > > > already used only for their side effects without a cast to void, e.g. > > > > > > /** > > > * @brief Default constructor creates an empty string. > > > */ > > > _GLIBCXX20_CONSTEXPR > > > basic_string() > > > _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) > > > : _M_dataplus(_M_local_data()) > > > { > > > _M_use_local_data(); > > > _M_set_length(0); > > > } > > > > > > I haven't updated these, but should this be changed for consistency? > > > > Yes, good idea. I can do that. > > I started to do that, and decided it made more sense to split out the > constexpr loop from _M_use_local_data() into a separate function, > _M_init_local_buf(). Then we can use that for all the places where we > don't care about the return value. That avoids the overhead of using > pointer_traits::pointer_to when we don't need the return value (which > is admittedly only going to be an issue for -O0 code, but I think it's > cleaner this way anyway). > > Please see the attached patch and let me know what you think. I agree, and it also looks clearer to me what is happening. > > > Thanks again for fixing these. I think this might fix some bug reports > > about clang rejecting our std::string in constant expressions, so I'll > > check those. > > Your patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110900 > (so we should backport it to gcc-13 and gcc-12 too). > commit 2668979d3206ff6c039ac0165aae29377a15666c > Author: Jonathan Wakely <jwakely@redhat.com> > Date: Fri Sep 29 12:12:22 2023 > > libstdc++: Split std::basic_string::_M_use_local_data into two functions > > This splits out the activate-the-union-member-for-constexpr logic from > _M_use_local_data, so that it can be used separately in cases that don't > need to use std::pointer_traits<pointer>::pointer_to to obtain the > return value. > > This leaves only three uses of _M_use_local_data() which are all the > same form: > > __s._M_data(_M_use_local_data()); > __s._M_set_length(0); > > We could remove _M_use_local_data() and change those three places to use > a new _M_reset() function that does: > > _M_init_local_buf(); > _M_data(_M_local_data()); > _M_set_length(0); > > This is left for a future change. > > libstdc++-v3/ChangeLog: > > * include/bits/basic_string.h (_M_init_local_buf()): New > function. > (_M_use_local_data()): Use _M_init_local_buf. > (basic_string(), basic_string(const Alloc&)) > (basic_string(basic_string&&)) > (basic_string(basic_string&&, const Alloc&)): Use > _M_init_local_buf instead of _M_use_local_data(). > * include/bits/basic_string.tcc (swap(basic_string&)) > (_M_construct(InIter, InIter, forward_iterator_tag)) > (_M_construct(size_type, CharT), reserve()): Likewise. > (_M_construct(InIter, InIter, input_iterator_tag)): Remove call > to _M_use_local_data() and initialize the local buffer directly. > > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h > index 4f94cd967cf..18a19b8dcbc 100644 > --- a/libstdc++-v3/include/bits/basic_string.h > +++ b/libstdc++-v3/include/bits/basic_string.h > @@ -353,13 +353,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > // Ensure that _M_local_buf is the active member of the union. > __attribute__((__always_inline__)) > _GLIBCXX14_CONSTEXPR > - pointer > - _M_use_local_data() _GLIBCXX_NOEXCEPT > + void > + _M_init_local_buf() _GLIBCXX_NOEXCEPT > { > #if __cpp_lib_is_constant_evaluated > if (std::is_constant_evaluated()) > for (size_type __i = 0; __i <= _S_local_capacity; ++__i) > _M_local_buf[__i] = _CharT(); > +#endif > + } > + > + __attribute__((__always_inline__)) > + _GLIBCXX14_CONSTEXPR > + pointer > + _M_use_local_data() _GLIBCXX_NOEXCEPT > + { > +#if __glibcxx_is_constant_evaluated > + _M_init_local_buf(); > #endif > return _M_local_data(); > } What's the difference between __cpp_lib_is_constant_evaluated and __glibcxx_is_constant_evaluated? Should these lines be using the same macro here? > @@ -522,7 +532,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) > : _M_dataplus(_M_local_data()) > { > - _M_use_local_data(); > + _M_init_local_buf(); > _M_set_length(0); > } > > @@ -534,7 +544,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > basic_string(const _Alloc& __a) _GLIBCXX_NOEXCEPT > : _M_dataplus(_M_local_data(), __a) > { > - _M_use_local_data(); > + _M_init_local_buf(); > _M_set_length(0); > } > > @@ -678,7 +688,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > { > if (__str._M_is_local()) > { > - (void)_M_use_local_data(); > + _M_init_local_buf(); > traits_type::copy(_M_local_buf, __str._M_local_buf, > __str.length() + 1); > } > @@ -718,7 +728,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > { > if (__str._M_is_local()) > { > - (void)_M_use_local_data(); > + _M_init_local_buf(); > traits_type::copy(_M_local_buf, __str._M_local_buf, > __str.length() + 1); > _M_length(__str.length()); > diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc > index 4bc98f2aea7..052eeb9e846 100644 > --- a/libstdc++-v3/include/bits/basic_string.tcc > +++ b/libstdc++-v3/include/bits/basic_string.tcc > @@ -79,7 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > else if (__s.length()) > { > - (void)_M_use_local_data(); > + _M_init_local_buf(); > traits_type::copy(_M_local_buf, __s._M_local_buf, > __s.length() + 1); > _M_length(__s.length()); > @@ -88,7 +88,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > } > else if (length()) > { > - (void)__s._M_use_local_data(); > + __s._M_init_local_buf(); > traits_type::copy(__s._M_local_buf, _M_local_buf, > length() + 1); > __s._M_length(length()); > @@ -99,7 +99,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > else > { > const size_type __tmp_capacity = __s._M_allocated_capacity; > - (void)__s._M_use_local_data(); > + __s._M_init_local_buf(); > traits_type::copy(__s._M_local_buf, _M_local_buf, > length() + 1); > _M_data(__s._M_data()); > @@ -111,7 +111,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > const size_type __tmp_capacity = _M_allocated_capacity; > if (__s._M_is_local()) > { > - (void)_M_use_local_data(); > + _M_init_local_buf(); > traits_type::copy(_M_local_buf, __s._M_local_buf, > __s.length() + 1); > __s._M_data(_M_data()); > @@ -174,14 +174,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > size_type __len = 0; > size_type __capacity = size_type(_S_local_capacity); > > - pointer __p = _M_use_local_data(); > - > while (__beg != __end && __len < __capacity) > { > - __p[__len++] = *__beg; > + _M_local_buf[__len++] = *__beg; > ++__beg; > } > > +#if __glibcxx_is_constant_evaluated > + if (std::is_constant_evaluated()) > + for (size_type __i = __len; __i <= __capacity; ++__i) > + _M_local_buf[__i] = _CharT(); > +#endif > + I wonder if maybe this should still be a call to `_M_init_local_buf()` above, where the `_M_use_local_data()` used to be? That way the logic stays in one place, and I don't imagine the compile time savings of not immediately overwriting the first __len characters would be significant. > struct _Guard > { > _GLIBCXX20_CONSTEXPR > @@ -230,7 +234,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > _M_capacity(__dnew); > } > else > - _M_use_local_data(); > + _M_init_local_buf(); > > // Check for out_of_range and length_error exceptions. > struct _Guard > @@ -263,7 +267,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > _M_capacity(__n); > } > else > - _M_use_local_data(); > + _M_init_local_buf(); > > if (__n) > this->_S_assign(_M_data(), __n, __c); > @@ -372,7 +376,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > if (__length <= size_type(_S_local_capacity)) > { > - this->_S_copy(_M_use_local_data(), _M_data(), __length + 1); > + _M_init_local_buf(); > + this->_S_copy(_M_local_buf, _M_data(), __length + 1); > _M_destroy(__capacity); > _M_data(_M_local_data()); > } But looks good to me; I tried applying this on top of my frontend patch and it looks like everything still works correctly. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libstdc++: Ensure active union member is correctly set 2023-09-29 16:29 ` Nathaniel Shead @ 2023-09-29 16:46 ` Jonathan Wakely 2023-10-21 14:45 ` Jonathan Wakely 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Wakely @ 2023-09-29 16:46 UTC (permalink / raw) To: Nathaniel Shead; +Cc: Jonathan Wakely, Jason Merrill, libstdc++, gcc-patches On Fri, 29 Sept 2023 at 17:29, Nathaniel Shead <nathanieloshead@gmail.com> wrote: > > On Fri, Sep 29, 2023 at 04:06:33PM +0100, Jonathan Wakely wrote: > > On Fri, 29 Sept 2023 at 10:32, Jonathan Wakely <jwakely@redhat.com> wrote: > > > > Thanks for the comments, here's an updated version of the patch. > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > > > Great, I'll get this committed today - thanks! > > > > That's done now. > > > > Thanks! > > > > > > > > > I'll note that there are some existing calls to `_M_use_local_data()` > > > > already used only for their side effects without a cast to void, e.g. > > > > > > > > /** > > > > * @brief Default constructor creates an empty string. > > > > */ > > > > _GLIBCXX20_CONSTEXPR > > > > basic_string() > > > > _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) > > > > : _M_dataplus(_M_local_data()) > > > > { > > > > _M_use_local_data(); > > > > _M_set_length(0); > > > > } > > > > > > > > I haven't updated these, but should this be changed for consistency? > > > > > > Yes, good idea. I can do that. > > > > I started to do that, and decided it made more sense to split out the > > constexpr loop from _M_use_local_data() into a separate function, > > _M_init_local_buf(). Then we can use that for all the places where we > > don't care about the return value. That avoids the overhead of using > > pointer_traits::pointer_to when we don't need the return value (which > > is admittedly only going to be an issue for -O0 code, but I think it's > > cleaner this way anyway). > > > > Please see the attached patch and let me know what you think. > > I agree, and it also looks clearer to me what is happening. Good, I'll make this change next week then. > > > > > > Thanks again for fixing these. I think this might fix some bug reports > > > about clang rejecting our std::string in constant expressions, so I'll > > > check those. > > > > Your patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110900 > > (so we should backport it to gcc-13 and gcc-12 too). > > > commit 2668979d3206ff6c039ac0165aae29377a15666c > > Author: Jonathan Wakely <jwakely@redhat.com> > > Date: Fri Sep 29 12:12:22 2023 > > > > libstdc++: Split std::basic_string::_M_use_local_data into two functions > > > > This splits out the activate-the-union-member-for-constexpr logic from > > _M_use_local_data, so that it can be used separately in cases that don't > > need to use std::pointer_traits<pointer>::pointer_to to obtain the > > return value. > > > > This leaves only three uses of _M_use_local_data() which are all the > > same form: > > > > __s._M_data(_M_use_local_data()); > > __s._M_set_length(0); > > > > We could remove _M_use_local_data() and change those three places to use > > a new _M_reset() function that does: > > > > _M_init_local_buf(); > > _M_data(_M_local_data()); > > _M_set_length(0); > > > > This is left for a future change. > > > > libstdc++-v3/ChangeLog: > > > > * include/bits/basic_string.h (_M_init_local_buf()): New > > function. > > (_M_use_local_data()): Use _M_init_local_buf. > > (basic_string(), basic_string(const Alloc&)) > > (basic_string(basic_string&&)) > > (basic_string(basic_string&&, const Alloc&)): Use > > _M_init_local_buf instead of _M_use_local_data(). > > * include/bits/basic_string.tcc (swap(basic_string&)) > > (_M_construct(InIter, InIter, forward_iterator_tag)) > > (_M_construct(size_type, CharT), reserve()): Likewise. > > (_M_construct(InIter, InIter, input_iterator_tag)): Remove call > > to _M_use_local_data() and initialize the local buffer directly. > > > > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h > > index 4f94cd967cf..18a19b8dcbc 100644 > > --- a/libstdc++-v3/include/bits/basic_string.h > > +++ b/libstdc++-v3/include/bits/basic_string.h > > @@ -353,13 +353,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > // Ensure that _M_local_buf is the active member of the union. > > __attribute__((__always_inline__)) > > _GLIBCXX14_CONSTEXPR > > - pointer > > - _M_use_local_data() _GLIBCXX_NOEXCEPT > > + void > > + _M_init_local_buf() _GLIBCXX_NOEXCEPT > > { > > #if __cpp_lib_is_constant_evaluated > > if (std::is_constant_evaluated()) > > for (size_type __i = 0; __i <= _S_local_capacity; ++__i) > > _M_local_buf[__i] = _CharT(); > > +#endif > > + } > > + > > + __attribute__((__always_inline__)) > > + _GLIBCXX14_CONSTEXPR > > + pointer > > + _M_use_local_data() _GLIBCXX_NOEXCEPT > > + { > > +#if __glibcxx_is_constant_evaluated > > + _M_init_local_buf(); > > #endif > > return _M_local_data(); > > } > > What's the difference between __cpp_lib_is_constant_evaluated and > __glibcxx_is_constant_evaluated? Should these lines be using the same > macro here? Ah, yeah, they could be the same. The difference is that (for most feature test macros) the __glibcxx_ftm form is defined after including <bits/version.h>, but the __cpp_lib_ftm form is only defined only by headers that do: #define __glibcxx_want_ftm #include <bits/version.h> This means we can ensure that the __cpp_lib_ftm form is only defined by headers where we actually want to define it, e.g. in the headers that [version.syn] in the standard says should define the macro. So for our own internal uses, we should generally rely on the __glibcxx_ftm one. Users should rely on __cpp_lib_ftm after including the correct header. For example, __glibcxx_atomic_wait is defined after including <bits/atomic_wait.h> and so is available for our own uses, but __cpp_lib_atomic_wait is only defined after including <atomic>, not just <bits/atomic_wait.h>. Or at least, that's the plan - I have a pending patch to make everything I just said true :-) Currently we "leak" the __cpp_lib_ftm forms into lots of <bits/xxx.h> internal headers. That will change next week. In this specific case, it doesn't matter, because __cpp_lib_is_constant_evaluated is defined by <type_traits>, and every header includes that. So it doesn't really matter whether our internal uses are __cpp_lib_is_constant_evaluated or __glibcxx_is_constant_evaluated. But it would be good to be consistent. > > > @@ -522,7 +532,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) > > : _M_dataplus(_M_local_data()) > > { > > - _M_use_local_data(); > > + _M_init_local_buf(); > > _M_set_length(0); > > } > > > > @@ -534,7 +544,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > basic_string(const _Alloc& __a) _GLIBCXX_NOEXCEPT > > : _M_dataplus(_M_local_data(), __a) > > { > > - _M_use_local_data(); > > + _M_init_local_buf(); > > _M_set_length(0); > > } > > > > @@ -678,7 +688,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > { > > if (__str._M_is_local()) > > { > > - (void)_M_use_local_data(); > > + _M_init_local_buf(); > > traits_type::copy(_M_local_buf, __str._M_local_buf, > > __str.length() + 1); > > } > > @@ -718,7 +728,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > { > > if (__str._M_is_local()) > > { > > - (void)_M_use_local_data(); > > + _M_init_local_buf(); > > traits_type::copy(_M_local_buf, __str._M_local_buf, > > __str.length() + 1); > > _M_length(__str.length()); > > diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc > > index 4bc98f2aea7..052eeb9e846 100644 > > --- a/libstdc++-v3/include/bits/basic_string.tcc > > +++ b/libstdc++-v3/include/bits/basic_string.tcc > > @@ -79,7 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > } > > else if (__s.length()) > > { > > - (void)_M_use_local_data(); > > + _M_init_local_buf(); > > traits_type::copy(_M_local_buf, __s._M_local_buf, > > __s.length() + 1); > > _M_length(__s.length()); > > @@ -88,7 +88,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > } > > else if (length()) > > { > > - (void)__s._M_use_local_data(); > > + __s._M_init_local_buf(); > > traits_type::copy(__s._M_local_buf, _M_local_buf, > > length() + 1); > > __s._M_length(length()); > > @@ -99,7 +99,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > else > > { > > const size_type __tmp_capacity = __s._M_allocated_capacity; > > - (void)__s._M_use_local_data(); > > + __s._M_init_local_buf(); > > traits_type::copy(__s._M_local_buf, _M_local_buf, > > length() + 1); > > _M_data(__s._M_data()); > > @@ -111,7 +111,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > const size_type __tmp_capacity = _M_allocated_capacity; > > if (__s._M_is_local()) > > { > > - (void)_M_use_local_data(); > > + _M_init_local_buf(); > > traits_type::copy(_M_local_buf, __s._M_local_buf, > > __s.length() + 1); > > __s._M_data(_M_data()); > > @@ -174,14 +174,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > size_type __len = 0; > > size_type __capacity = size_type(_S_local_capacity); > > > > - pointer __p = _M_use_local_data(); > > - > > while (__beg != __end && __len < __capacity) > > { > > - __p[__len++] = *__beg; > > + _M_local_buf[__len++] = *__beg; > > ++__beg; > > } > > > > +#if __glibcxx_is_constant_evaluated > > + if (std::is_constant_evaluated()) > > + for (size_type __i = __len; __i <= __capacity; ++__i) > > + _M_local_buf[__i] = _CharT(); > > +#endif > > + > > I wonder if maybe this should still be a call to `_M_init_local_buf()` > above, where the `_M_use_local_data()` used to be? That way the logic > stays in one place, and I don't imagine the compile time savings of not > immediately overwriting the first __len characters would be significant. Yeah, I went back and forth on that, but you're probably right. I'll do as you suggested. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] libstdc++: Ensure active union member is correctly set 2023-09-29 16:46 ` Jonathan Wakely @ 2023-10-21 14:45 ` Jonathan Wakely 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Wakely @ 2023-10-21 14:45 UTC (permalink / raw) To: Jonathan Wakely; +Cc: Nathaniel Shead, Jason Merrill, libstdc++, gcc-patches [-- Attachment #1: Type: text/plain, Size: 11322 bytes --] On Fri, 29 Sept 2023 at 17:46, Jonathan Wakely <jwakely.gcc@gmail.com> wrote: > > On Fri, 29 Sept 2023 at 17:29, Nathaniel Shead > <nathanieloshead@gmail.com> wrote: > > > > On Fri, Sep 29, 2023 at 04:06:33PM +0100, Jonathan Wakely wrote: > > > On Fri, 29 Sept 2023 at 10:32, Jonathan Wakely <jwakely@redhat.com> wrote: > > > > > Thanks for the comments, here's an updated version of the patch. > > > > > Bootstrapped and regtested on x86_64-pc-linux-gnu. > > > > > > > > Great, I'll get this committed today - thanks! > > > > > > That's done now. > > > > > > > Thanks! > > > > > > > > > > > > I'll note that there are some existing calls to `_M_use_local_data()` > > > > > already used only for their side effects without a cast to void, e.g. > > > > > > > > > > /** > > > > > * @brief Default constructor creates an empty string. > > > > > */ > > > > > _GLIBCXX20_CONSTEXPR > > > > > basic_string() > > > > > _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) > > > > > : _M_dataplus(_M_local_data()) > > > > > { > > > > > _M_use_local_data(); > > > > > _M_set_length(0); > > > > > } > > > > > > > > > > I haven't updated these, but should this be changed for consistency? > > > > > > > > Yes, good idea. I can do that. > > > > > > I started to do that, and decided it made more sense to split out the > > > constexpr loop from _M_use_local_data() into a separate function, > > > _M_init_local_buf(). Then we can use that for all the places where we > > > don't care about the return value. That avoids the overhead of using > > > pointer_traits::pointer_to when we don't need the return value (which > > > is admittedly only going to be an issue for -O0 code, but I think it's > > > cleaner this way anyway). > > > > > > Please see the attached patch and let me know what you think. > > > > I agree, and it also looks clearer to me what is happening. > > Good, I'll make this change next week then. > > > > > > > > > > > Thanks again for fixing these. I think this might fix some bug reports > > > > about clang rejecting our std::string in constant expressions, so I'll > > > > check those. > > > > > > Your patch fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110900 > > > (so we should backport it to gcc-13 and gcc-12 too). > > > > > commit 2668979d3206ff6c039ac0165aae29377a15666c > > > Author: Jonathan Wakely <jwakely@redhat.com> > > > Date: Fri Sep 29 12:12:22 2023 > > > > > > libstdc++: Split std::basic_string::_M_use_local_data into two functions > > > > > > This splits out the activate-the-union-member-for-constexpr logic from > > > _M_use_local_data, so that it can be used separately in cases that don't > > > need to use std::pointer_traits<pointer>::pointer_to to obtain the > > > return value. > > > > > > This leaves only three uses of _M_use_local_data() which are all the > > > same form: > > > > > > __s._M_data(_M_use_local_data()); > > > __s._M_set_length(0); > > > > > > We could remove _M_use_local_data() and change those three places to use > > > a new _M_reset() function that does: > > > > > > _M_init_local_buf(); > > > _M_data(_M_local_data()); > > > _M_set_length(0); > > > > > > This is left for a future change. > > > > > > libstdc++-v3/ChangeLog: > > > > > > * include/bits/basic_string.h (_M_init_local_buf()): New > > > function. > > > (_M_use_local_data()): Use _M_init_local_buf. > > > (basic_string(), basic_string(const Alloc&)) > > > (basic_string(basic_string&&)) > > > (basic_string(basic_string&&, const Alloc&)): Use > > > _M_init_local_buf instead of _M_use_local_data(). > > > * include/bits/basic_string.tcc (swap(basic_string&)) > > > (_M_construct(InIter, InIter, forward_iterator_tag)) > > > (_M_construct(size_type, CharT), reserve()): Likewise. > > > (_M_construct(InIter, InIter, input_iterator_tag)): Remove call > > > to _M_use_local_data() and initialize the local buffer directly. > > > > > > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h > > > index 4f94cd967cf..18a19b8dcbc 100644 > > > --- a/libstdc++-v3/include/bits/basic_string.h > > > +++ b/libstdc++-v3/include/bits/basic_string.h > > > @@ -353,13 +353,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > > // Ensure that _M_local_buf is the active member of the union. > > > __attribute__((__always_inline__)) > > > _GLIBCXX14_CONSTEXPR > > > - pointer > > > - _M_use_local_data() _GLIBCXX_NOEXCEPT > > > + void > > > + _M_init_local_buf() _GLIBCXX_NOEXCEPT > > > { > > > #if __cpp_lib_is_constant_evaluated > > > if (std::is_constant_evaluated()) > > > for (size_type __i = 0; __i <= _S_local_capacity; ++__i) > > > _M_local_buf[__i] = _CharT(); > > > +#endif > > > + } > > > + > > > + __attribute__((__always_inline__)) > > > + _GLIBCXX14_CONSTEXPR > > > + pointer > > > + _M_use_local_data() _GLIBCXX_NOEXCEPT > > > + { > > > +#if __glibcxx_is_constant_evaluated > > > + _M_init_local_buf(); > > > #endif > > > return _M_local_data(); > > > } > > > > What's the difference between __cpp_lib_is_constant_evaluated and > > __glibcxx_is_constant_evaluated? Should these lines be using the same > > macro here? > > Ah, yeah, they could be the same. > > The difference is that (for most feature test macros) the > __glibcxx_ftm form is defined after including <bits/version.h>, but > the __cpp_lib_ftm form is only defined only by headers that do: > > #define __glibcxx_want_ftm > #include <bits/version.h> > > This means we can ensure that the __cpp_lib_ftm form is only defined > by headers where we actually want to define it, e.g. in the headers > that [version.syn] in the standard says should define the macro. So > for our own internal uses, we should generally rely on the > __glibcxx_ftm one. Users should rely on __cpp_lib_ftm after including > the correct header. For example, __glibcxx_atomic_wait is defined > after including <bits/atomic_wait.h> and so is available for our own > uses, but __cpp_lib_atomic_wait is only defined after including > <atomic>, not just <bits/atomic_wait.h>. Or at least, that's the plan > - I have a pending patch to make everything I just said true :-) > Currently we "leak" the __cpp_lib_ftm forms into lots of <bits/xxx.h> > internal headers. That will change next week. > > In this specific case, it doesn't matter, because > __cpp_lib_is_constant_evaluated is defined by <type_traits>, and every > header includes that. So it doesn't really matter whether our internal > uses are __cpp_lib_is_constant_evaluated or > __glibcxx_is_constant_evaluated. But it would be good to be > consistent. > > > > > > @@ -522,7 +532,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > > _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) > > > : _M_dataplus(_M_local_data()) > > > { > > > - _M_use_local_data(); > > > + _M_init_local_buf(); > > > _M_set_length(0); > > > } > > > > > > @@ -534,7 +544,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > > basic_string(const _Alloc& __a) _GLIBCXX_NOEXCEPT > > > : _M_dataplus(_M_local_data(), __a) > > > { > > > - _M_use_local_data(); > > > + _M_init_local_buf(); > > > _M_set_length(0); > > > } > > > > > > @@ -678,7 +688,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > > { > > > if (__str._M_is_local()) > > > { > > > - (void)_M_use_local_data(); > > > + _M_init_local_buf(); > > > traits_type::copy(_M_local_buf, __str._M_local_buf, > > > __str.length() + 1); > > > } > > > @@ -718,7 +728,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 > > > { > > > if (__str._M_is_local()) > > > { > > > - (void)_M_use_local_data(); > > > + _M_init_local_buf(); > > > traits_type::copy(_M_local_buf, __str._M_local_buf, > > > __str.length() + 1); > > > _M_length(__str.length()); > > > diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc > > > index 4bc98f2aea7..052eeb9e846 100644 > > > --- a/libstdc++-v3/include/bits/basic_string.tcc > > > +++ b/libstdc++-v3/include/bits/basic_string.tcc > > > @@ -79,7 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > } > > > else if (__s.length()) > > > { > > > - (void)_M_use_local_data(); > > > + _M_init_local_buf(); > > > traits_type::copy(_M_local_buf, __s._M_local_buf, > > > __s.length() + 1); > > > _M_length(__s.length()); > > > @@ -88,7 +88,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > } > > > else if (length()) > > > { > > > - (void)__s._M_use_local_data(); > > > + __s._M_init_local_buf(); > > > traits_type::copy(__s._M_local_buf, _M_local_buf, > > > length() + 1); > > > __s._M_length(length()); > > > @@ -99,7 +99,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > else > > > { > > > const size_type __tmp_capacity = __s._M_allocated_capacity; > > > - (void)__s._M_use_local_data(); > > > + __s._M_init_local_buf(); > > > traits_type::copy(__s._M_local_buf, _M_local_buf, > > > length() + 1); > > > _M_data(__s._M_data()); > > > @@ -111,7 +111,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > const size_type __tmp_capacity = _M_allocated_capacity; > > > if (__s._M_is_local()) > > > { > > > - (void)_M_use_local_data(); > > > + _M_init_local_buf(); > > > traits_type::copy(_M_local_buf, __s._M_local_buf, > > > __s.length() + 1); > > > __s._M_data(_M_data()); > > > @@ -174,14 +174,18 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION > > > size_type __len = 0; > > > size_type __capacity = size_type(_S_local_capacity); > > > > > > - pointer __p = _M_use_local_data(); > > > - > > > while (__beg != __end && __len < __capacity) > > > { > > > - __p[__len++] = *__beg; > > > + _M_local_buf[__len++] = *__beg; > > > ++__beg; > > > } > > > > > > +#if __glibcxx_is_constant_evaluated > > > + if (std::is_constant_evaluated()) > > > + for (size_type __i = __len; __i <= __capacity; ++__i) > > > + _M_local_buf[__i] = _CharT(); > > > +#endif > > > + > > > > I wonder if maybe this should still be a call to `_M_init_local_buf()` > > above, where the `_M_use_local_data()` used to be? That way the logic > > stays in one place, and I don't imagine the compile time savings of not > > immediately overwriting the first __len characters would be significant. > > > Yeah, I went back and forth on that, but you're probably right. I'll > do as you suggested. Here's what I've pushed to trunk after testing on x86_64-linux. Thanks again for the fixes. [-- Attachment #2: patch.txt --] [-- Type: text/plain, Size: 6006 bytes --] commit 405a4140fc30bce86b1ec885a98bb17704e0c8c6 Author: Jonathan Wakely <jwakely@redhat.com> Date: Fri Sep 29 12:12:22 2023 libstdc++: Split std::basic_string::_M_use_local_data into two functions This splits out the activate-the-union-member-for-constexpr logic from _M_use_local_data, so that it can be used separately in cases that don't need to use std::pointer_traits<pointer>::pointer_to to obtain the return value. This leaves only three uses of _M_use_local_data() which are all of the same form: __s._M_data(_M_use_local_data()); __s._M_set_length(0); We could remove _M_use_local_data() and change those three places to use a new _M_reset() function that does: _M_init_local_buf(); _M_data(_M_local_data()); _M_set_length(0); This is left for a future change. libstdc++-v3/ChangeLog: * include/bits/basic_string.h (_M_init_local_buf()): New function. (_M_use_local_data()): Use _M_init_local_buf. (basic_string(), basic_string(const Alloc&)) (basic_string(basic_string&&)) (basic_string(basic_string&&, const Alloc&)): Use _M_init_local_buf instead of _M_use_local_data(). * include/bits/basic_string.tcc (swap(basic_string&)) (_M_construct(InIter, InIter, input_iterator_tag)) (_M_construct(InIter, InIter, forward_iterator_tag)) (_M_construct(size_type, CharT), reserve()): Likewise. diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h index 4f94cd967cf..0fa32afeb84 100644 --- a/libstdc++-v3/include/bits/basic_string.h +++ b/libstdc++-v3/include/bits/basic_string.h @@ -353,13 +353,23 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 // Ensure that _M_local_buf is the active member of the union. __attribute__((__always_inline__)) _GLIBCXX14_CONSTEXPR - pointer - _M_use_local_data() _GLIBCXX_NOEXCEPT + void + _M_init_local_buf() _GLIBCXX_NOEXCEPT { #if __cpp_lib_is_constant_evaluated if (std::is_constant_evaluated()) for (size_type __i = 0; __i <= _S_local_capacity; ++__i) _M_local_buf[__i] = _CharT(); +#endif + } + + __attribute__((__always_inline__)) + _GLIBCXX14_CONSTEXPR + pointer + _M_use_local_data() _GLIBCXX_NOEXCEPT + { +#if __cpp_lib_is_constant_evaluated + _M_init_local_buf(); #endif return _M_local_data(); } @@ -522,7 +532,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 _GLIBCXX_NOEXCEPT_IF(is_nothrow_default_constructible<_Alloc>::value) : _M_dataplus(_M_local_data()) { - _M_use_local_data(); + _M_init_local_buf(); _M_set_length(0); } @@ -534,7 +544,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 basic_string(const _Alloc& __a) _GLIBCXX_NOEXCEPT : _M_dataplus(_M_local_data(), __a) { - _M_use_local_data(); + _M_init_local_buf(); _M_set_length(0); } @@ -678,7 +688,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { if (__str._M_is_local()) { - (void)_M_use_local_data(); + _M_init_local_buf(); traits_type::copy(_M_local_buf, __str._M_local_buf, __str.length() + 1); } @@ -718,7 +728,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11 { if (__str._M_is_local()) { - (void)_M_use_local_data(); + _M_init_local_buf(); traits_type::copy(_M_local_buf, __str._M_local_buf, __str.length() + 1); _M_length(__str.length()); diff --git a/libstdc++-v3/include/bits/basic_string.tcc b/libstdc++-v3/include/bits/basic_string.tcc index 4bc98f2aea7..f0a44e5e881 100644 --- a/libstdc++-v3/include/bits/basic_string.tcc +++ b/libstdc++-v3/include/bits/basic_string.tcc @@ -79,7 +79,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } else if (__s.length()) { - (void)_M_use_local_data(); + _M_init_local_buf(); traits_type::copy(_M_local_buf, __s._M_local_buf, __s.length() + 1); _M_length(__s.length()); @@ -88,7 +88,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION } else if (length()) { - (void)__s._M_use_local_data(); + __s._M_init_local_buf(); traits_type::copy(__s._M_local_buf, _M_local_buf, length() + 1); __s._M_length(length()); @@ -99,7 +99,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION else { const size_type __tmp_capacity = __s._M_allocated_capacity; - (void)__s._M_use_local_data(); + __s._M_init_local_buf(); traits_type::copy(__s._M_local_buf, _M_local_buf, length() + 1); _M_data(__s._M_data()); @@ -111,7 +111,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION const size_type __tmp_capacity = _M_allocated_capacity; if (__s._M_is_local()) { - (void)_M_use_local_data(); + _M_init_local_buf(); traits_type::copy(_M_local_buf, __s._M_local_buf, __s.length() + 1); __s._M_data(_M_data()); @@ -174,11 +174,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION size_type __len = 0; size_type __capacity = size_type(_S_local_capacity); - pointer __p = _M_use_local_data(); + _M_init_local_buf(); while (__beg != __end && __len < __capacity) { - __p[__len++] = *__beg; + _M_local_buf[__len++] = *__beg; ++__beg; } @@ -230,7 +230,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_capacity(__dnew); } else - _M_use_local_data(); + _M_init_local_buf(); // Check for out_of_range and length_error exceptions. struct _Guard @@ -263,7 +263,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION _M_capacity(__n); } else - _M_use_local_data(); + _M_init_local_buf(); if (__n) this->_S_assign(_M_data(), __n, __c); @@ -372,7 +372,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION if (__length <= size_type(_S_local_capacity)) { - this->_S_copy(_M_use_local_data(), _M_data(), __length + 1); + _M_init_local_buf(); + this->_S_copy(_M_local_buf, _M_data(), __length + 1); _M_destroy(__capacity); _M_data(_M_local_data()); } ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-10-21 14:45 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <ZO30PQql2TablzpJ@Thaum.localdomain> [not found] ` <053faf76-f918-7527-4a41-755a18d0018a@redhat.com> [not found] ` <ZPHXiqquRCNCREoX@Thaum.localdomain> [not found] ` <e5fb1597-28a9-1be6-f914-5ed475732da2@redhat.com> [not found] ` <ZQpDAkaSSdkc0Q+R@Thaum.localdomain> [not found] ` <e11829bb-68c2-98b6-c7d0-9f9dbd58cb25@redhat.com> [not found] ` <ZQxIGK4oIxNMun2i@Thaum.localdomain> [not found] ` <bfe816fe-5ce5-8dc8-3d00-455ef37d67df@redhat.com> [not found] ` <ZQ2sQNoVz0rblzDN@Thaum.localdomain> 2023-09-23 0:38 ` [PATCH v4] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286] 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
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).