[PATCH] D107087: Fix SectionMemoryManager deconstruction error with MSVC

[Justice Adams via Phabricator via llvm-commits]

justice_adams created this revision.
justice_adams added reviewers: RKSimon, lhames, kazu.
justice_adams added a project: LLVM.
Herald added a subscriber: hiraditya.
justice_adams requested review of this revision.

This commit fixes an issue with deconstructing the `SectionMemoryManager` where the `_vfptr` on the `MMapper` instance variable would be wiped out by the time the `~SectionMemoryManager()`
was called, thus causing a method call to `_purecall()` (MSVC specific) which then would trigger an `abort()`

This is most evident when running the LLVM Kaleidoscope example on windows.

  Destructors of objects with static storage duration are called in reverse order of completion of their constructors or the completion of their dynamic initialization, and the functions passed to std::atexit are called in reverse order they are registered (last one first)

See https://en.cppreference.com/w/cpp/utility/program/exit

Previously, If a user created an object of static storage duration containing a `SectionMemoryManager` before the `DefaultMMapperInstance` was instantiated, this will cause an error. For example, consider the following construction order in a program
1.) `static std::unique_ptr<KaleidoscopeJIT>` (which holds an `ObjectLayer` which has a `SectionMemoryManager`)
2.) `DefaultMMapperInstance`

Thus, at deconstruction time the order is
1.) `DefaultMMapperInstance` (This wipes out the Vtable)
2.) `static std::unique_ptr<KaleidoscopeJIT>` (which holds an `ObjectLayer` which has a `SectionMemoryManager`)

when the second destructor is called, it calls the virtual method on a wiped out table. Thus the explosion.

The solution here is to make `SectionMemoryManager` not rely on a static variable which can be wiped out before an instance of the class is ready to be destroyed, and thus ensuring each `SectionMemoryManager` has it's own reliable instance of `DefaultMMapperInstance` to be used at deconstruction time.

Note this bug error is specific to MSVC.

I have confirmed all tests pass on both Windows and Linux


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107087

Files:
  llvm/include/llvm/ExecutionEngine/SectionMemoryManager.h
  llvm/lib/ExecutionEngine/SectionMemoryManager.cpp


Index: llvm/lib/ExecutionEngine/SectionMemoryManager.cpp
===================================================================
--- llvm/lib/ExecutionEngine/SectionMemoryManager.cpp
+++ llvm/lib/ExecutionEngine/SectionMemoryManager.cpp
@@ -242,31 +242,6 @@
 
 void SectionMemoryManager::anchor() {}
 
-namespace {
-// Trivial implementation of SectionMemoryManager::MemoryMapper that just calls
-// into sys::Memory.
-class DefaultMMapper final : public SectionMemoryManager::MemoryMapper {
-public:
-  sys::MemoryBlock
-  allocateMappedMemory(SectionMemoryManager::AllocationPurpose Purpose,
-                       size_t NumBytes, const sys::MemoryBlock *const NearBlock,
-                       unsigned Flags, std::error_code &EC) override {
-    return sys::Memory::allocateMappedMemory(NumBytes, NearBlock, Flags, EC);
-  }
-
-  std::error_code protectMappedMemory(const sys::MemoryBlock &Block,
-                                      unsigned Flags) override {
-    return sys::Memory::protectMappedMemory(Block, Flags);
-  }
-
-  std::error_code releaseMappedMemory(sys::MemoryBlock &M) override {
-    return sys::Memory::releaseMappedMemory(M);
-  }
-};
-
-DefaultMMapper DefaultMMapperInstance;
-} // namespace
-
 SectionMemoryManager::SectionMemoryManager(MemoryMapper *MM)
     : MMapper(MM ? *MM : DefaultMMapperInstance) {}
 
Index: llvm/include/llvm/ExecutionEngine/SectionMemoryManager.h
===================================================================
--- llvm/include/llvm/ExecutionEngine/SectionMemoryManager.h
+++ llvm/include/llvm/ExecutionEngine/SectionMemoryManager.h
@@ -149,6 +149,28 @@
   /// This method is called from finalizeMemory.
   virtual void invalidateInstructionCache();
 
+  // Trivial implementation of SectionMemoryManager::MemoryMapper that just
+  // calls into sys::Memory.
+  class DefaultMMapper final : public SectionMemoryManager::MemoryMapper {
+  public:
+    sys::MemoryBlock
+    allocateMappedMemory(SectionMemoryManager::AllocationPurpose Purpose,
+                         size_t NumBytes,
+                         const sys::MemoryBlock *const NearBlock,
+                         unsigned Flags, std::error_code &EC) override {
+      return sys::Memory::allocateMappedMemory(NumBytes, NearBlock, Flags, EC);
+    }
+
+    std::error_code protectMappedMemory(const sys::MemoryBlock &Block,
+                                        unsigned Flags) override {
+      return sys::Memory::protectMappedMemory(Block, Flags);
+    }
+
+    std::error_code releaseMappedMemory(sys::MemoryBlock &M) override {
+      return sys::Memory::releaseMappedMemory(M);
+    }
+  };
+
 private:
   struct FreeMemBlock {
     // The actual block of free memory
@@ -186,6 +208,7 @@
   MemoryGroup RWDataMem;
   MemoryGroup RODataMem;
   MemoryMapper &MMapper;
+  DefaultMMapper DefaultMMapperInstance;
 };
 
 } // end namespace llvm


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D107087.362805.patch
Type: text/x-patch
Size: 2873 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210729/09ae9aeb/attachment.bin>