[Bug c++/114245] New: Defaulted virtual destructors that do no work overwrite the vtable with `-O0`

[Bug c++/114245] New: Defaulted virtual destructors that do no work overwrite the vtable with `-O0`

[mwinkler at blizzard dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114245

            Bug ID: 114245
           Summary: Defaulted virtual destructors that do no work
                    overwrite the vtable with `-O0`
           Product: gcc
           Version: 13.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: mwinkler at blizzard dot com
  Target Milestone: ---

https://godbolt.org/z/Ge135h6qh

Use the godbolt for reference against all the 3 major compilers,
GCC/Clang/MSVC.
The following bug report is only for `-O0` builds.
Optimized builds end up removing the dead vtable stores but we require our
debug builds to behave the same as release builds.

Given a virtual destructor that is defaulted and does no work GCC will
overwrite the vtable with the current objects most constructed type.

This behaviour isn't required by MSVC or Itanium CXX ABI or the C++ std but it
would be nice to have all 3 major compilers agree here especially since MSVC
and Clang both have this behaviour.

```
struct Base { virtual ~Base() = default; };
struct Derived : Base { virtual ~Derived() = default; };
```

In the example above `~Derived` will overwrite the vtable with the vtable for
Derived. `~Base` will overwrite the vtable with the vtable for Base.

If you have a global object of `Derived` you easily run into static de-init
fiasco issues since these objects are also registered with `__cxa_atexit`.

Clang and MSVC for defaulted virtual destructors do not overwrite the vtable in
the generated destructors even though they still register with `__cxa_atexit`
so the running of the destructor ends up being a nop.
Clang also has the `[[clang::no_destroy]]` attribute.

We have a workaround on GCC by doing the following for such global objects.
```
union NoDestroy
{
    Derived v;
    constexpr NoDestroy() : v() {}
    ~NoDestroy() {}
};
```

There are two solutions that will work for our use case.

Add a similar attribute like `[[clang::no_destroy]]` to mark certain global
objects that should not be registered with `__cxa_atexit`. I couldn't find such
a similar attribute in the GCC docs.

Have GCC behave like MSVC and Clang here when optimizations are disabled and
have defaulted virtual destructors not overwrite the vtable.

Thanks :).

[Bug c++/114245] Defaulted virtual destructors that do no work overwrite the vtable with `-O0`

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114245

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 57622
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57622&action=edit
Full testcase

Next time please attach or paste inline the full testcase rather than just
linking to godbolt.

[Bug c++/114245] Defaulted virtual destructors that do no work overwrite the vtable with `-O0`

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114245

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I am trying to understand the issue of having the vtable writing to the object.

Is the issue you have an order issue where an object is destroyed but still in
use else where, that sounds like an undefined behavior in your code.

>you easily run into static de-init fiasco issues 

Yes this sounds like you are runing into what I have described as being
undefined behavior. Changing GCC here just works around the broken code which I
doubt is a good idea. If you need a specific order, across TUs, well C++ does
not define them.

GCC does have an init_priority attribute which could be used here to fix the
issue as earlier initializations are deconstructed later.

[Bug c++/114245] Defaulted virtual destructors that do no work overwrite the vtable with `-O0`

[mwinkler at blizzard dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114245

--- Comment #3 from Max Enrico Winkler <mwinkler at blizzard dot com> ---
> Yes this sounds like you are runing into what I have described as being undefined behavior.

Understood and agreed.

However this specific example is an object that effectively does no work on
destruction.
We would expect the object to be embedded in the binary and not rely on static
de-init order since there should be nothing running during destruction.

With `-O2` the vtable writes get dead store removed and the destructor ends up
being a full nop as expected.

Clang and MSVC have this behaviour and clang also provides more control with
the `no_destroy` attribute.

In general we didn't expect to have static de-init issues with an object of
this type since we didn't expect anything to be running during destruction.

We are aware of the various ways to work around static [de]-init issues but I
ultimately think GCC just shouldn't be overwriting vtables on destruction here.

If you guys think otherwise no worries we can work-around it.

[Bug c++/114245] Defaulted virtual destructors that do no work overwrite the vtable with `-O0`

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114245

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Updating the vtable is necessary during destruction if a later (i.e. less
derived) destructor calls a virtual function. But if we can tell that the
current dtor and all later base dtors are trivial, then we know that can't
happen.

If the front end detected that case, we wouldn't need to rely on dead store
elimination.

Maybe we could even avoid the __cxa_atexit registration if the most derived
dtor is trivial (I don't recall if there's some ABI reason we need that
registration even if the dtor is known to be a no-op). That is something that
that would be much harder for the middle end to do, so is an argument for the
front end handling it.

If we are allowed to remove the __cxa_atexit call, that changes this from
"remove some dead stores even at -O0 which helps some programs with UB" to a
more significant optimization.

[Bug c++/114245] Defaulted virtual destructors that do no work overwrite the vtable with `-O0`

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114245

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #4)
> Updating the vtable is necessary during destruction if a later (i.e. less
> derived) destructor calls a virtual function. But if we can tell that the
> current dtor and all later base dtors are trivial, then we know that can't
> happen.
> 
> If the front end detected that case, we wouldn't need to rely on dead store
> elimination.

But isn't that just an optimization the user asked not to perform (with using
-O0)?
I mean, e.g. the user could care about the vtable updates for e.g. calling
methods
from the debugger as the destruction of the object progresses.

[Bug c++/114245] Defaulted virtual destructors that do no work overwrite the vtable with `-O0`

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114245

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=115050
         Resolution|---                         |WONTFIX
             Status|UNCONFIRMED                 |RESOLVED

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
So I just debugged a bug report where the vtable storing in the deconstructor
found the bug in their code (PR 115050). It just happens with that store, it
causes a seg fault at runtime. Without the store the code seemly worked but
only on accident and I had to show that a deconstructor was called twice.

Having this store is definitely helpful for debugging code at -O0 and catching
undefined code early on.

So closing as won't fix. removing the vtable store is an optimization rather
than needed for correctness and having the vtable store there is very very
useful for debugabity of programs.