On 1/18/25 4:04 AM, Jakub Jelinek wrote:
On Fri, Jan 17, 2025 at 07:00:33PM -0500, Jason Merrill wrote:--- gcc/c-family/c-common.cc.jj 2025-01-02 11:47:29.803228077 +0100 +++ gcc/c-family/c-common.cc 2025-01-17 13:32:53.512294482 +0100 @@ -9016,9 +9016,26 @@ vec<tree, va_gc> * make_tree_vector_from_ctor (tree ctor) { vec<tree,va_gc> *ret = make_tree_vector (); + unsigned nelts = CONSTRUCTOR_NELTS (ctor); vec_safe_reserve (ret, CONSTRUCTOR_NELTS (ctor)); for (unsigned i = 0; i < CONSTRUCTOR_NELTS (ctor); ++i) - ret->quick_push (CONSTRUCTOR_ELT (ctor, i)->value); + if (TREE_CODE (CONSTRUCTOR_ELT (ctor, i)->value) == RAW_DATA_CST) + { + tree raw_data = CONSTRUCTOR_ELT (ctor, i)->value; + nelts += RAW_DATA_LENGTH (raw_data); + vec_safe_reserve (ret, nelts);I notice that the second argument to vec_safe_reserve is the number of empty spaces to provide, rather than the new desired capacity. So this will allocate space for an additional copy of the whole constructor.You're right.I guess you want to say reserve (ret, nelts - vec_safe_length (res))? It's weird that there isn't a more convenient way to express that.I think vec_safe_reserve (ret, nelts - ret->length ()); is enough, the first vec_safe_reserve already necessarily make ret non-NULL because if it would not have reserved anything (nelts == 0), then there would be no ctor elements to walk.
Agreed.
Note that this behavior differs from the C++ standard library, where reserve indeed specifies the new capacity. This seems likely to cause a lot of confusion (and over-allocation) going forward. And indeed looking through exising uses turns up at least two that seem to assume the standard library semantics (as well as others that assume the actual semantics).Which ones did you find? To me cp/name-lookup,cc (name_lookup::preserve_state) unsigned length = vec_safe_length (previous->scopes); vec_safe_reserve (previous->scopes, length * 2); looks possibly unintended, as it quick_pushes at most length further elements. I've so far skimmed just vec_safe_reserve uses in {c-family,c,cp} subdirectories, there are 26 further vec_safe_reserve and around 500 .reserve or ->reserve cases. Most of the vec_safe_reserve and reserve calls are on empty vectors so it really doesn't matter which one is which. So, I wonder if at least during the conversion it wouldn't be best to have all 3 namings, reserve, reserve_space and reserve_capacity, where the first one would at runtime assert that length () is 0.I wonder about renaming the existing reserve to reserve_space and adding a reserve_capacity?Would it be ok to first handle these PRs (fixed patch below) and go through the renaming and checking incrementally?
Absolutely.
There was another thinko in the patch, nelts += RAW_DATA_LENGTH (raw_data); would mean again reserving too much, it needs to be nelts += RAW_DATA_LENGTH (raw_data) - 1, as we don't push the RAW_DATA_CST itself which was already accounted for, but replace it with RAW_DATA_LENGTH pushes. So far tested on GXX_TESTSUITE_STDS=98,11,14,17,20,23,26 make check-g++ RUNTESTFLAGS="dg.exp='embed* class-deduction-aggr*.C explicit*.C pr11853[24]*'" ok if it passes full bootstrap+regtest?
OK.
2025-01-18 Jakub Jelinek <ja...@redhat.com> PR c++/118528 * c-common.cc (make_tree_vector_from_ctor): Expand RAW_DATA_CST elements from the CONSTRUCTOR to individual INTEGER_CSTs. * g++.dg/cpp/embed-21.C: New test. * g++.dg/cpp2a/class-deduction-aggr16.C: New test. --- gcc/c-family/c-common.cc.jj 2025-01-02 11:47:29.803228077 +0100 +++ gcc/c-family/c-common.cc 2025-01-17 13:32:53.512294482 +0100 @@ -9016,9 +9016,26 @@ vec<tree, va_gc> * make_tree_vector_from_ctor (tree ctor) { vec<tree,va_gc> *ret = make_tree_vector (); + unsigned nelts = CONSTRUCTOR_NELTS (ctor); vec_safe_reserve (ret, CONSTRUCTOR_NELTS (ctor)); for (unsigned i = 0; i < CONSTRUCTOR_NELTS (ctor); ++i) - ret->quick_push (CONSTRUCTOR_ELT (ctor, i)->value); + if (TREE_CODE (CONSTRUCTOR_ELT (ctor, i)->value) == RAW_DATA_CST) + { + tree raw_data = CONSTRUCTOR_ELT (ctor, i)->value; + nelts += RAW_DATA_LENGTH (raw_data) - 1; + vec_safe_reserve (ret, nelts - ret->length ()); + if (TYPE_PRECISION (TREE_TYPE (raw_data)) > CHAR_BIT + || TYPE_UNSIGNED (TREE_TYPE (raw_data))) + for (unsigned j = 0; j < (unsigned) RAW_DATA_LENGTH (raw_data); ++j) + ret->quick_push (build_int_cst (TREE_TYPE (raw_data), + RAW_DATA_UCHAR_ELT (raw_data, j))); + else + for (unsigned j = 0; j < (unsigned) RAW_DATA_LENGTH (raw_data); ++j) + ret->quick_push (build_int_cst (TREE_TYPE (raw_data), + RAW_DATA_SCHAR_ELT (raw_data, j))); + } + else + ret->quick_push (CONSTRUCTOR_ELT (ctor, i)->value); return ret; }--- gcc/testsuite/g++.dg/cpp/embed-21.C.jj 2025-01-17 12:18:00.571912195 +0100+++ gcc/testsuite/g++.dg/cpp/embed-21.C 2025-01-17 13:37:31.478489868 +0100 @@ -0,0 +1,22 @@ +// PR c++/118528 +// { dg-do compile { target c++20 } } +// { dg-options "" } + +template<class T> +struct E { T t[130][2]; }; + +E e1 { +#embed __FILE__ limit (260) +}; + +template<class T> +struct F { T t[2][130]; }; + +F f1 { +#embed __FILE__ limit (260) +}; +F f2 { { { +#embed __FILE__ limit (130) +}, { +#embed __FILE__ limit (130) +} } }; --- gcc/testsuite/g++.dg/cpp2a/class-deduction-aggr16.C.jj 2025-01-17 12:17:23.715424611 +0100 +++ gcc/testsuite/g++.dg/cpp2a/class-deduction-aggr16.C 2025-01-17 12:17:43.717146527 +0100 @@ -0,0 +1,17 @@ +// PR c++/118528 +// { dg-do compile { target c++20 } } + +template<class T> +struct E { T t[130][2]; }; + +#define P 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16 +#define Q { 1, 2 }, { 3, 4 }, { 5, 6 }, { 7, 8 }, { 9, 10 }, { 11, 12 }, \ + { 13, 14 }, { 15, 16 } +E e1 { P, P, P, P, P, P, P, P, P, P, P, P, P, P, P, P, 1, 2, 3, 4 }; +E e2 { { Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, Q, { 1, 2 }, { 3, 4 } } }; + +template<class T> +struct F { T t[2][130]; }; + +F f1 { P, P, P, P, P, P, P, P, P, P, P, P, P, P, P, P, 1, 2, 3, 4 }; +F f2 { { { P, P, P, P, P, P, P, P, 1, 2 }, { P, P, P, P, P, P, P, P, 3, 4 } } }; Jakub
