Skip to content

Commit

Permalink
Fix ManagedStatic SIOF for MSVC
Browse files Browse the repository at this point in the history
Summary:
The MSVC STL implementation of `std::atomic` has a non-trivial but
`constexpr` default constructor, because it value initialises the
member. On older versions of MSVC there was a bug in the compiler
implementation where `constexpr` constructors would generate dynamic
code and so a workaround was added to the STL to make the
`std::atomic<T>` default constructor trivial (by removing value
initialisation of `T`).

This has been fine so far because the bug and workaround have
remained. However, in a recent version of MSVC, the bug was fixed and
the `std::atomic` constructor was returned to being non-trivial.

With this change, `ManagedStatic` now has a non-trivial,
non-constexpr, default constructor, since it attempts to zero
initialise `Ptr`. This means that `ManagedStatic` is being
dynamically initialised. This leads to the following initialisation
order problem:

`ManagedStatic` lazily initialises its `Ptr` member when it is first
used. If `ManagedStatic` gets used before it has been initialised
(say, in the constructor of some other static), then the `Ptr` member
will first get set, and then subsequently zeroed out.

This breaks the LLVM command line parser, which relies on
`ManagedStatic`. It also breaks Hermes builds, because we run
`hermesc` during the build process to compile the internal bytecode.

The fix is to just add a `constexpr` constructor for `ManagedStatic`
for new versions of MSVC, and non-MSVC compilers (for when they are
using the MSVC STL). However, because of the issue with `constexpr`
constructors on old versions of MSVC, keep the old behaviour there.

This diff copies https://reviews.llvm.org/D80433 to LLVH.

Reviewed By: tmikov

Differential Revision: D25139806

fbshipit-source-id: 8f0b97a6f5824dca39acf3a1cb96a6fa4c7f6b02
  • Loading branch information
neildhar authored and facebook-github-bot committed Nov 21, 2020
1 parent 396a281 commit c810466
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 0 deletions.
23 changes: 23 additions & 0 deletions external/llvh/include/llvh/Support/ManagedStatic.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,41 @@ template <typename T, size_t N> struct object_deleter<T[N]> {
static void call(void *Ptr) { delete[](T *)Ptr; }
};

// ManagedStatic must be initialized to zero, and it must *not* have a dynamic
// initializer because managed statics are often created while running other
// dynamic initializers. In standard C++11, the best way to accomplish this is
// with a constexpr default constructor. However, different versions of the
// Visual C++ compiler have had bugs where, even though the constructor may be
// constexpr, a dynamic initializer may be emitted depending on optimization
// settings. For the affected versions of MSVC, use the old linker
// initialization pattern of not providing a constructor and leaving the fields
// uninitialized. See http://llvm.org/PR41367 for details.
#if !defined(_MSC_VER) || (_MSC_VER >= 1925) || defined(__clang__)
#define LLVM_USE_CONSTEXPR_CTOR
#endif

/// ManagedStaticBase - Common base class for ManagedStatic instances.
class ManagedStaticBase {
protected:
#ifdef LLVM_USE_CONSTEXPR_CTOR
mutable std::atomic<void *> Ptr{};
mutable void (*DeleterFn)(void *) = nullptr;
mutable const ManagedStaticBase *Next = nullptr;
#else
// This should only be used as a static variable, which guarantees that this
// will be zero initialized.
mutable std::atomic<void *> Ptr;
mutable void (*DeleterFn)(void*);
mutable const ManagedStaticBase *Next;
#endif

void RegisterManagedStatic(void *(*creator)(), void (*deleter)(void*)) const;

public:
#ifdef LLVM_USE_CONSTEXPR_CTOR
constexpr ManagedStaticBase() = default;
#endif

/// isConstructed - Return true if this object has not been created yet.
bool isConstructed() const { return Ptr != nullptr; }

Expand Down
45 changes: 45 additions & 0 deletions external/llvh/patches/ManagedStaticSIOF.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
diff --git a/xplat/hermes/external/llvh/include/llvh/Support/ManagedStatic.h b/xplat/hermes/external/llvh/include/llvh/Support/ManagedStatic.h
--- a/xplat/hermes/external/llvh/include/llvh/Support/ManagedStatic.h
+++ b/xplat/hermes/external/llvh/include/llvh/Support/ManagedStatic.h
@@ -33,18 +33,41 @@
static void call(void *Ptr) { delete[](T *)Ptr; }
};

+// ManagedStatic must be initialized to zero, and it must *not* have a dynamic
+// initializer because managed statics are often created while running other
+// dynamic initializers. In standard C++11, the best way to accomplish this is
+// with a constexpr default constructor. However, different versions of the
+// Visual C++ compiler have had bugs where, even though the constructor may be
+// constexpr, a dynamic initializer may be emitted depending on optimization
+// settings. For the affected versions of MSVC, use the old linker
+// initialization pattern of not providing a constructor and leaving the fields
+// uninitialized. See http://llvm.org/PR41367 for details.
+#if !defined(_MSC_VER) || (_MSC_VER >= 1925) || defined(__clang__)
+#define LLVM_USE_CONSTEXPR_CTOR
+#endif
+
/// ManagedStaticBase - Common base class for ManagedStatic instances.
class ManagedStaticBase {
protected:
+#ifdef LLVM_USE_CONSTEXPR_CTOR
+ mutable std::atomic<void *> Ptr{};
+ mutable void (*DeleterFn)(void *) = nullptr;
+ mutable const ManagedStaticBase *Next = nullptr;
+#else
// This should only be used as a static variable, which guarantees that this
// will be zero initialized.
mutable std::atomic<void *> Ptr;
mutable void (*DeleterFn)(void*);
mutable const ManagedStaticBase *Next;
+#endif

void RegisterManagedStatic(void *(*creator)(), void (*deleter)(void*)) const;

public:
+#ifdef LLVM_USE_CONSTEXPR_CTOR
+ constexpr ManagedStaticBase() = default;
+#endif
+
/// isConstructed - Return true if this object has not been created yet.
bool isConstructed() const { return Ptr != nullptr; }

0 comments on commit c810466

Please sign in to comment.