[Bug libstdc++/105880] New: eh_globals_init destructor not setting _M_init to false

[Bug libstdc++/105880] New: eh_globals_init destructor not setting _M_init to false

[chrisj at rtems dot org]

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

            Bug ID: 105880
           Summary: eh_globals_init destructor not setting _M_init to
                    false
           Product: gcc
           Version: 12.1.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: chrisj at rtems dot org
  Target Milestone: ---

RTEMS 6 with GCC 12 (gcc version 12.1.1 20220509 (RTEMS 6, RSB
e73a258a3aa4af8735b589a2d770571b2105ac5f, Newlib 64b2081) (GCC)) is calling
`std::terminate()` in `__cxa_get_globals()`. The `_M_init` value is `true`
after the static destructor has run.

RTEMS is tracking the issue with https://devel.rtems.org/ticket/4661.

When `exit()` is called the static destructor for `eh_gloabls init` runs and
deletes the POSIX key however the `_M_init` variable is not set to false.

Later `__cxxabiv1::__cxa_get_globals()` is called and sees `_M_init` is `true`
so attempts to get the POSIX key value. This fails because the key has been
deleted so `NULL` is returned. Memory is allocated and the set fails because
there is no key so `std::terminate()` is called.

The issue is `init._M_init` is not set to false in the static destructor. The
code for aarch64 and arm is:

006562e0 <__eh_globals_init::~__eh_globals_init()>:
  6562e0:       7903            ldrb    r3, [r0, #4]
  6562e2:       b510            push    {r4, lr}
  6562e4:       4604            mov     r4, r0
  6562e6:       b90b            cbnz    r3, 6562ec
<__eh_globals_init::~__eh_globals_init()+0xc>
  6562e8:       4620            mov     r0, r4
  6562ea:       bd10            pop     {r4, pc}
  6562ec:       6800            ldr     r0, [r0, #0]
  6562ee:       f04d fe6b       bl      6a3fc8 <pthread_key_delete>
  6562f2:       4620            mov     r0, r4
  6562f4:       bd10            pop     {r4, pc}

0000000010222c30 <__eh_globals_init::~__eh_globals_init()>:
    10222c30:   39401001        ldrb    w1, [x0, #4]
    10222c34:   35000041        cbnz    w1, 10222c3c
<__eh_globals_init::~__eh_globals_init()+0xc>
    10222c38:   d65f03c0        ret
    10222c3c:   a9bf7bfd        stp     x29, x30, [sp, #-16]!
    10222c40:   910003fd        mov     x29, sp
  return pthread_key_delete (__key);
    10222c44:   b9400000        ldr     w0, [x0]
    10222c48:   94008806        bl      10244c60 <pthread_key_delete>
    10222c4c:   a8c17bfd        ldp     x29, x30, [sp], #16
    10222c50:   d65f03c0        ret

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[sebastian.huber@embedded-brains.de]

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

Sebastian Huber <sebastian.huber@embedded-brains.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sebastian.huber@embedded-br
                   |                            |ains.de

--- Comment #1 from Sebastian Huber <sebastian.huber@embedded-brains.de> ---
Just for reference, the destructor code is (eh_globals.cc):

struct __eh_globals_init
{
  __gthread_key_t       _M_key;
  bool                  _M_init;

[...]

  ~__eh_globals_init()
  {
    if (_M_init)
      __gthread_key_delete(_M_key);
    _M_init = false; <-- This store is optimized away
  }
};

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Sebastian Huber from comment #1)
> Just for reference, the destructor code is (eh_globals.cc):
> 
> struct __eh_globals_init
> {
>   __gthread_key_t  	_M_key;
>   bool 			_M_init;
> 
> [...]
> 
>   ~__eh_globals_init()
>   {
>     if (_M_init)
>       __gthread_key_delete(_M_key);
>     _M_init = false; <-- This store is optimized away
>   }
> };

Because it is dead after the deconstruct is called.

What is the full sequence is there another constructor being called after exit?
Or something else?

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Sounds like the order of deconstructors is wrong.
Where is __cxxabiv1::__cxa_get_globals being called from that is the problem?

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[chrisj at rtems dot org]

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

--- Comment #4 from Chris Johns <chrisj at rtems dot org> ---
(In reply to Andrew Pinski from comment #3)
> Sounds like the order of deconstructors is wrong.
> Where is __cxxabiv1::__cxa_get_globals being called from that is the problem?

The `std::ios_base::Init::~Init()` is being called after this object has
destructed.

The ios_base class uses a sentry to check the state. The code in it's
destructor is:

if (bool(_M_os.flags() & ios_base::unitbuf) && !uncaught_exception())  

The std::cerr object has ios_base::unitbuf set so uncaught_exception() is
called. This call gets the cxa globals:

__cxa_eh_globals *globals = __cxa_get_globals ();

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[pinskia at gcc dot gnu.org]

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

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
There has to be an ordering issue of the calling of the deconstructors vs the
ordering of the constructors.

Maybe the problem is the `eh_gloabls init` priority is not set correctly to be
first/last.

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[chrisj at rtems dot org]

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

--- Comment #6 from Chris Johns <chrisj at rtems dot org> ---
(In reply to Andrew Pinski from comment #5)
> There has to be an ordering issue of the calling of the deconstructors vs
> the ordering of the constructors.
> 
> Maybe the problem is the `eh_gloabls init` priority is not set correctly to
> be first/last.

It would have to be last or close to it and I have no idea how that could be
done without additional attributes that are not standard.

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[redi at gcc dot gnu.org]

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

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Chris Johns from comment #6)
> It would have to be last or close to it and I have no idea how that could be
> done without additional attributes that are not standard.

That's fine, the C++ runtime doesn't have to be written in standard C++, it's
only compiled by GCC. The init_priority attribute could be used here, but it's
not supported on all targets.

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[redi at gcc dot gnu.org]

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

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
As Andrew says, the problem here is that __cxa_get_globals is being used after
the global has been destroyed. Nothing done to the _M_init member in the
destructor can be changed to fix that, accessing an object after its destructor
runs is undefined, period.

What might "work" is to make the _M_init member a static data member, which
outlives the singleton instance of the class.

Something like this:

--- a/libstdc++-v3/libsupc++/eh_globals.cc
+++ b/libstdc++-v3/libsupc++/eh_globals.cc
@@ -90,29 +90,34 @@ eh_globals_dtor(void* ptr)
 struct __eh_globals_init
 {
   __gthread_key_t      _M_key;
-  bool                         _M_init;
+  static bool          _S_init;

-  __eh_globals_init() : _M_init(false)
-  { 
+  __eh_globals_init()
+  {
     if (__gthread_active_p())
-      _M_init = __gthread_key_create(&_M_key, eh_globals_dtor) == 0; 
+      _S_init = __gthread_key_create(&_M_key, eh_globals_dtor) == 0;
   }

   ~__eh_globals_init()
   {
-    if (_M_init)
+    if (_S_init)
       __gthread_key_delete(_M_key);
-    _M_init = false;
+    _S_init = false;
   }
+
+  __eh_globals_init(const __eh_globals_init&) = delete;
+  __eh_globals_init& operator=(const __eh_globals_init&) = delete;
 };

+bool __eh_globals_init::_S_init = false;
+
 static __eh_globals_init init;

 extern "C" __cxa_eh_globals*
 __cxxabiv1::__cxa_get_globals_fast() _GLIBCXX_NOTHROW
 {
   __cxa_eh_globals* g;
-  if (init._M_init)
+  if (init._S_init)
     g = static_cast<__cxa_eh_globals*>(__gthread_getspecific(init._M_key));
   else
     g = &eh_globals;
@@ -123,7 +128,7 @@ extern "C" __cxa_eh_globals*
 __cxxabiv1::__cxa_get_globals() _GLIBCXX_NOTHROW
 {
   __cxa_eh_globals* g;
-  if (init._M_init)
+  if (init._S_init)
     {
       g = static_cast<__cxa_eh_globals*>(__gthread_getspecific(init._M_key));
       if (!g)


This doesn't really seem correct though, as it just means we revert to the
eh_globals single-threaded fallback buffer after _S_init becomes false. But
that fallback buffer has also been destroyed at that point. So maybe we need to
make eh_globals immortal.

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[redi at gcc dot gnu.org]

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

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Created attachment 53103
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53103&action=edit
Fix lifetime bugs for non-TLS eh_globals

Does this work?

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[chrisj at rtems dot org]

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

--- Comment #10 from Chris Johns <chrisj at rtems dot org> ---
(In reply to Jonathan Wakely from comment #9)
> Created attachment 53103 [details]
> Fix lifetime bugs for non-TLS eh_globals
> 
> Does this work?

That is a great way to solve this problem given the limitation the eh_globals
has. 

And yes the eh_globals single-threaded fallback buffer does have this problem
however for RTEMS that is a corner case because something has called `exit()`
on an embedded realtime system and that typically means a reset is about to
happen.

FYI Sebastian has reviewed our compiler set up as TLS was not enabled. The
architectures that support TLS should soon be switching to that.

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2022-06-09
             Status|UNCONFIRMED                 |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Chris Johns from comment #10)
> And yes the eh_globals single-threaded fallback buffer does have this
> problem however for RTEMS that is a corner case because something has called
> `exit()` on an embedded realtime system and that typically means a reset is
> about to happen.

Right. Nobody is going to be creating new threads and throwing exceptions in a
function registered with atexit!

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:1e65f2ed99024f23c56f7b6a961898bcaa882a92

commit r13-1039-g1e65f2ed99024f23c56f7b6a961898bcaa882a92
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Jun 8 10:43:57 2022 +0100

    libstdc++: Fix lifetime bugs for non-TLS eh_globals [PR105880]

    This ensures that the single-threaded fallback buffer eh_globals is not
    destroyed during program termination, using the same immortalization
    technique used for error category objects.

    Also ensure that init._M_init can still be read after init has been
    destroyed, by making it a static data member.

    libstdc++-v3/ChangeLog:

            PR libstdc++/105880
            * libsupc++/eh_globals.cc (eh_globals): Ensure constant init and
            prevent destruction during termination.
            (__eh_globals_init::_M_init): Replace with static member _S_init.
            (__cxxabiv1::__cxa_get_globals_fast): Update.
            (__cxxabiv1::__cxa_get_globals): Likewise.

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
   Target Milestone|---                         |13.0
             Status|ASSIGNED                    |RESOLVED

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This should be fixed now, please reopen if not.

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[cvs-commit at gcc dot gnu.org]

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

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Sebastian Huber
<sh@gcc.gnu.org>:

https://gcc.gnu.org/g:ade3197134cc9ed2b7e589a7b58c73110ae13f1c

commit r12-8589-gade3197134cc9ed2b7e589a7b58c73110ae13f1c
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Wed Jun 8 10:43:57 2022 +0100

    libstdc++: Fix lifetime bugs for non-TLS eh_globals [PR105880]

    This ensures that the single-threaded fallback buffer eh_globals is not
    destroyed during program termination, using the same immortalization
    technique used for error category objects.

    Also ensure that init._M_init can still be read after init has been
    destroyed, by making it a static data member.

    libstdc++-v3/ChangeLog:

            PR libstdc++/105880
            * libsupc++/eh_globals.cc (eh_globals): Ensure constant init and
            prevent destruction during termination.
            (__eh_globals_init::_M_init): Replace with static member _S_init.
            (__cxxabiv1::__cxa_get_globals_fast): Update.
            (__cxxabiv1::__cxa_get_globals): Likewise.

    (cherry picked from commit 1e65f2ed99024f23c56f7b6a961898bcaa882a92)

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.0                        |12.2

--- Comment #15 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Backported for 12.2 now.

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[redi at gcc dot gnu.org]

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

--- Comment #16 from Jonathan Wakely <redi at gcc dot gnu.org> ---
For the record, r12-8589-gade3197134cc9e was needed to fix something in the
commits above, and was also backported to gcc-12.

[Bug libstdc++/105880] eh_globals_init destructor not setting _M_init to false

[rdiezmail-gcc at yahoo dot de]

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

R. Diez <rdiezmail-gcc at yahoo dot de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rdiezmail-gcc at yahoo dot de

--- Comment #17 from R. Diez <rdiezmail-gcc at yahoo dot de> ---
For the record, this fix introduces a call to atexit() for a static destructor,
see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107500