Hi,
On 24 Jan 2025, at 17:50, Jason Merrill wrote:
> On 1/23/25 12:20 PM, Simon Martin wrote:
>> 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?
>
> Well, I'm not sure there is such a testcase. The earlier checks skip
> actual initializers, while the later checks skip members without
> corresponding initializers. So if we see a vptr initializer and no
> longer skip it early, as long as its initializer's position in the
> CONSTRUCTOR matches the position of the vptr in TYPE_FIELDS, we'll
> continue through the loop normally.
>
> The case where this could cause a problem would be if the CONSTRUCTOR
> order doesn't match the TYPE_FIELDS order for some reason. I think
> that this shouldn't ever happen currently, because a class with
> virtual bases can't have a constexpr constructor, and non-virtual
> bases are laid out in order. If C++ ever supports constexpr virtual
> bases, we'll need to handle them specially here.
>
> So actually I think your change is fine. I'd just clarify the comment
> to say "Skip vptr adjustment represented with a COMPONENT_REF."
>
> OK with that change.
Thanks Jason. Pushed to trunk as r15-7209-gbee1910b891f89; I don’t
plan to backport.
Simon
