Hi Jason,
On 21 Jan 2025, at 22:51, Jason Merrill wrote:
> On 1/3/25 3:00 PM, Simon Martin wrote:
>> We currently accept this code with c++ <= 17 even though it's invalid
>> since the base is not initialized (we properly reject it with c++ >=
>> 20)
>>
>> === cut here ===
>> struct NoMut1 { int a, b; };
>> struct NoMut3 : NoMut1 {
>> constexpr NoMut3(int a, int b) {}
>> };
>> void mutable_subobjects() {
>> constexpr NoMut3 nm3(1, 2);
>> }
>> === cut here ===
>>
>> This is a fallout of r0-118700-gc2b3ec18a494e3, that ignores all
>> fields
>> with DECL_ARTIFICIAL in cx_check_missing_mem_inits, including those
>> that
>> represent base classes, and need to be checked.
>>
>> This patch makes sure that we only skip fields that have
>> DECL_ARTIFICIAL
>> if they don't have DECL_FIELD_IS_BASE.
>>
>> Successfully tested on x86_64-pc-linux-gnu.
>>
>> PR c++/118239
>>
>> gcc/cp/ChangeLog:
>>
>> * constexpr.cc (cx_check_missing_mem_inits): Don't skip fields
>> with DECL_FIELD_IS_BASE.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * g++.dg/cpp0x/constexpr-base8.C: New test.
>>
>> ---
>> gcc/cp/constexpr.cc | 8 +++----
>> gcc/testsuite/g++.dg/cpp0x/constexpr-base8.C | 24
>> ++++++++++++++++++++
>> 2 files changed, 28 insertions(+), 4 deletions(-)
>> create mode 100644 gcc/testsuite/g++.dg/cpp0x/constexpr-base8.C
>>
>> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
>> index c8be5a525ee..e11444438d3 100644
>> --- a/gcc/cp/constexpr.cc
>> +++ b/gcc/cp/constexpr.cc
>> @@ -839,9 +839,8 @@ cx_check_missing_mem_inits (tree ctype, tree
>> body, bool complain)
>> if (i < nelts)
>> {
>> index = CONSTRUCTOR_ELT (body, i)->index;
>> - /* Skip base and vtable inits. */
>> - if (TREE_CODE (index) != FIELD_DECL
>> - || DECL_ARTIFICIAL (index))
>> + /* Skip vtable inits. */
>> + if (TREE_CODE (index) != FIELD_DECL)
>> continue;
>
> The code after your change no longer skips vtable inits; this should
> make the same DECL_FIELD_IS_BASE adjustment as below.
Ouch, thanks for catching this! I hate that I could break things like
this, so I tried to craft a test case that would have failed, but was
not able to so far :-(
I can see that you added this DECL_ARTIFICIAL check via
93a85785e0d08d149077f342201b9952a84669a7, and obviously the tests that
you’d added at that time worked even with my mistake. Do you have any
suggestion about what kind of construct could hit this code I broke, so
that I add a test case along with my next patch revision?
Thanks!
Simon
>
>> }
>> @@ -852,7 +851,8 @@ cx_check_missing_mem_inits (tree ctype, tree
>> body, bool complain)
>> continue;
>> if (DECL_UNNAMED_BIT_FIELD (field))
>> continue;
>> - if (DECL_ARTIFICIAL (field))
>> + /* Artificial fields can be ignored unless they're bases. */
>> + if (DECL_ARTIFICIAL (field) && !DECL_FIELD_IS_BASE (field))
>> continue;
>> if (ANON_AGGR_TYPE_P (TREE_TYPE (field)))
>> {
>> diff --git a/gcc/testsuite/g++.dg/cpp0x/constexpr-base8.C
>> b/gcc/testsuite/g++.dg/cpp0x/constexpr-base8.C
>> new file mode 100644
>> index 00000000000..ecc28693315
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/cpp0x/constexpr-base8.C
>> @@ -0,0 +1,24 @@
>> +// PR c++/118239
>> +// { dg-do "compile" { target c++11 } }
>> +
>> +struct NoMut1 {
>> + int a, b;
>> +};
>> +
>> +// Reported case.
>> +struct NoMut2 : NoMut1 {
>> + constexpr NoMut2(int a, int b) /*: NoMut1()*/
>> + {} // { dg-error "must be initialized" "" { target c++17_down } }
>> +};
>> +
>> +// Variant with explicit initialization of some member.
>> +struct NoMut3 : NoMut1 {
>> + constexpr NoMut3(int a, int b) : c(0) /*, NoMut1()*/
>> + {} // { dg-error "must be initialized" "" { target c++17_down } }
>> + int c;
>> +};
>> +
>> +void mutable_subobjects() {
>> + constexpr NoMut2 nm2(1, 2); // { dg-error "constant expression" }
>> + constexpr NoMut3 nm3(1, 2); // { dg-error "constant expression" }
>> +}
