[RFC] libstdc++: optional: Set _M_engaged after the call to destructor [PR127005]

[Jonathan Wakely]

On Mon, 24 Aug 2026 at 02:02, Andrea Pinski
<andrew.pinski@oss.qualcomm.com> wrote:
>
> On Sun, Aug 23, 2026 at 4:41 PM Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> >
> >
> >
> > On Sun, 23 Aug 2026, 20:26 Andrea Pinski, <andrew.pinski@oss.qualcomm.com> wrote:
> >>
> >> Sometimes the destructor will cause the optimizers not to see that
> >> _M_engaged has been to set to false (e.g. an external call). This
> >> means that we might get an uninitialized warning after the destructor
> >> in some cases.
> >> Setting it after the destructor will cause one extra store in some cases
> >> but that store allows the optimizers to get rid of other code.
> >>
> >> This is a RFC because I don't know if we want to remove the first store
> >> of _M_engaged or keep it.
> >> The patch has been bootstrapped and tested on x86_64-linux-gnu with no regressions.
> >> And tested provided in the bug report works and also the generated code looks much
> >> better too.
> >>
> >>         PR libstdc++/127005
> >>
> >> libstdc++-v3/ChangeLog:
> >>
> >>         * include/std/optional (optional::_M_destroy): Set _M_engaged
> >>         to false after the destructor call.
> >>
> >> Signed-off-by: Andrea Pinski <andrew.pinski@oss.qualcomm.com>
> >> ---
> >>  libstdc++-v3/include/std/optional | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/libstdc++-v3/include/std/optional b/libstdc++-v3/include/std/optional
> >> index d6fd33e8c2e..8884c54f3e7 100644
> >> --- a/libstdc++-v3/include/std/optional
> >> +++ b/libstdc++-v3/include/std/optional
> >> @@ -303,6 +303,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> >>        {
> >>         _M_engaged = false;
> >>         _M_payload._M_value.~_Stored_type();
> >> +       _M_engaged = false;
> >
> >
> > Do you see the same benefits from
> >
> > if (_M_engaged)
> >   __builtin_unreachable();
> >
> > ?
>
> Yes. that works here too. Yes I agree this version is better so I will
> go and test this version tomorrow and submit with a full patch
> including a testcase.

Thanks. I think I prefer to mark it unreachable, that makes it more
clear that we don't expect the _M_engaged flag to be changed while
destroying the contained object (because it's UB for the object
destructor to even read that flag, let alone do something that changes
it).

Setting it before and after the destructor seems to imply that maybe
it's OK for it to change, as though we're intentionally supporting for
a valid scenario.

>
>
> >
> >>  #if defined(__clang__) && __cpp_lib_optional >= 202106L // full constexpr support
> >>         if (std::is_constant_evaluated())
> >>           // Work around PR124910 for Clang.
> >> --
> >> 2.43.0
> >>
>