Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thread-safe static initialization for interrupts and FreeRTOS #345

Closed
salkinium opened this issue Mar 12, 2020 · 3 comments
Closed

Thread-safe static initialization for interrupts and FreeRTOS #345

salkinium opened this issue Mar 12, 2020 · 3 comments

Comments

@salkinium
Copy link
Member

salkinium commented Mar 12, 2020

During #343, I discovered that we compile with -fno-threadsafe-statics.
Background information (these are some good reads):

When compiling without this flag, the default implementation of stdlibc++ is used, which compiles to this:

0x8003158 <__cxa_guard_acquire>         ldr    r3, [r0, #0]
0x800315a <__cxa_guard_acquire+2>       lsls   r3, r3, #31
0x800315c <__cxa_guard_acquire+4>       bmi.n  0x800316c <__cxa_guard_acquire+20>
0x800315e <__cxa_guard_acquire+6>       ldrb   r3, [r0, #1]
0x8003160 <__cxa_guard_acquire+8>       cbz    r3, 0x8003164 <__cxa_guard_acquire+12>
0x8003162 <__cxa_guard_acquire+10>      udf    #255    ; 0xff
0x8003164 <__cxa_guard_acquire+12>      movs   r3, #1
0x8003166 <__cxa_guard_acquire+14>      strb   r3, [r0, #1]
0x8003168 <__cxa_guard_acquire+16>      mov    r0, r3
0x800316a <__cxa_guard_acquire+18>      bx     lr
0x800316c <__cxa_guard_acquire+20>      movs   r0, #0
0x800316e <__cxa_guard_acquire+22>      bx     lr
0x8003170 <__cxa_guard_release>         movs   r3, #1
0x8003172 <__cxa_guard_release+2>       str    r3, [r0, #0]
0x8003174 <__cxa_guard_release+4>       bx     lr

which if you compare this to the source code and mentally resolve all necessary #defines, is basically this simple implementation:

  // The ARM EABI uses the least significant bit of a 32-bit
  // guard variable.  */
#define _GLIBCXX_GUARD_TEST(x) ((*(x) & 1) != 0)
#define _GLIBCXX_GUARD_SET(x) *(x) = 1
#define _GLIBCXX_GUARD_BIT 1
#define _GLIBCXX_GUARD_PENDING_BIT __guard_test_bit (1, 1)
#define _GLIBCXX_GUARD_WAITING_BIT __guard_test_bit (2, 1)
  typedef int __guard;

# undef _GLIBCXX_GUARD_TEST_AND_ACQUIRE
# undef _GLIBCXX_GUARD_SET_AND_RELEASE
# define _GLIBCXX_GUARD_SET_AND_RELEASE(G) _GLIBCXX_GUARD_SET (G)

  static inline int
  init_in_progress_flag(__guard* g)
  { return ((char *)g)[1]; }

  static inline void
  set_init_in_progress_flag(__guard* g, int v)
  { ((char *)g)[1] = v; }

  static inline void
  throw_recursive_init_exception()
  {
#if __cpp_exceptions
   throw __gnu_cxx::recursive_init_error();
#else
   // Use __builtin_trap so we don't require abort().
   __builtin_trap();
#endif
  }

  // acquire() is a helper function used to acquire guard if thread support is
  // not compiled in or is compiled in but not enabled at run-time.
  static int
  acquire(__guard *g)
  {
    // Quit if the object is already initialized.
    if (_GLIBCXX_GUARD_TEST(g))
      return 0;

    if (init_in_progress_flag(g))
      throw_recursive_init_exception();

    set_init_in_progress_flag(g, 1);
    return 1;
  }

  extern "C"
  int __cxa_guard_acquire (__guard *g)
  {
    return acquire (g);
  }

  extern "C"
  void __cxa_guard_release (__guard *g) throw ()
  {
    set_init_in_progress_flag(g, 0);
    _GLIBCXX_GUARD_SET_AND_RELEASE (g);
  }

This traps recursive initialization, however, it is neither atomic nor interrupt-safe on Cortex-M where interrupts can be nested! I would assume that accessing the guard atomically would suffice to make it interrupt safe for plain code, I don't think we need to lock the entire initialization procedure (you could rely on interrupts inside a constructor by logging for example).

However, when using the FreeRTOS module (or any other RTOS), static initialization needs to additionally be guarded by a mutex to prevent context switching during initialization?

cc @chris-durand @rleh @dergraaf

@salkinium
Copy link
Member Author

salkinium commented Mar 12, 2020

Additional headaches: I've created a simple example that recursively initializes a static class:

void constructDummy();
class Dummy
{
public:
    Dummy()
    {
        MODM_LOG_INFO << "Dummy class constructed" << modm::endl;
        constructDummy(); // recursive initialization
    }
};
void constructDummy()
{
    static Dummy dummy;
    MODM_LOG_INFO << "constructDummy() called" << modm::endl;
}

With -fno-threadsafe-statics this enteres an infinite loop. That's bad.
Without this flag, the stdlibc++ implementation (as above) calls __builtin_trap() which is implemented as udf #255, ie. calling an undefined instruction, which causes a hardfault. This is really, really unhelpful, since by default the HardFault handler is not hooked and :platform:fault is not included either by default, so the processor simply reboots (silently).

For that alone, we should overwrite this terrible default implementation.

@phillipjohnston
Copy link

I use compare-and-swap intrinsics here:

https://github.com/embeddedartistry/libcpp/blob/master/src/lightabi/cxa_guard.cpp

@salkinium
Copy link
Member Author

I use compare-and-swap intrinsics here:

I checked carefully, and I'm fairly certain that this is also how I'm doing it, minus the support for thread-safety at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants