On Sat, Jan 18, 2025 at 1:02 AM Jason Merrill <ja...@redhat.com> wrote: > > On 1/17/25 3:08 PM, Jakub Jelinek wrote: > > Hi! > > > > This is the first bug discovered today with the > > https://gcc.gnu.org/pipermail/gcc-patches/2025-January/673945.html > > hack but then turned into proper testcases where embed-21.C FAILed > > since introduction of optimized #embed support and the other when > > optimizing large C++ initializers using RAW_DATA_CST. > > > > The problem is that the C++ FE calls make_tree_vector_from_ctor > > and uses that as arguments vector for deduction guide handling. > > The call.cc code isn't prepared to handle RAW_DATA_CST just about > > everywhere, so I think it is safer to make sure RAW_DATA_CST only > > appears in CONSTRUCTOR_ELTS and nowhere else. > > Thus, the following patch expands the RAW_DATA_CSTs from initializers > > into multiple INTEGER_CSTs in the returned vector. > > > > Bootstrapped on x86_64-linux and i686-linux, regtests pending, ok > > for trunk if it passes? > > > > 2025-01-17 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); > > + 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. > > 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. > > 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). > > I wonder about renaming the existing reserve to reserve_space and adding > a reserve_capacity?
To me that's equally confusing. To me our ::reserve semantics is sound, but I can see if you know std::vector by heard that you're easily confused. IMO std::vector::reserve (1) doing nothing when .length () == 2 is confusing, I asked it to reserve some space! So, can we fix the standard library instead? (OK, just joking) We do have the issue of branch maintainance when doing any change here, so we can't change the semantic of ::reserve but we'd have to remove it. I'll note we have ::space to query free capacity while the standard library only has ::capacity. So iff then we should simply rename ::reserve to ::reserve_space, but not adding another (confusing) variant. Richard. > > Jason >
