Fix SectionMemoryManager deconstruction error with MSVC

[justice_adams]

Summary

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

Update: I have updated the diff to use ManagedStatic per recommendation and confirmed it fixes the issue

[lhames]

Hi Justice!

Thanks very much for identifying the problem here.

I think it feels odd to make DefaultMMapper a member of SectionMemoryManager. Does it fix your issue if you wrap the static global in a ManagedStatic instead? (see https://llvm.org/docs/ProgrammersManual.html#lazy-initialization-with-managedstatic).

• Lang.

[justice_adams]

@lhames You are correct! I wasn't familiar with ManagedStatic but that does fix it. Do you prefer that approach to what's here? If so, I'll go ahead and update the diff. Originally I thought it made sense to have DefaultMMapperInstance be an instance variable of the SectionMemoryManager since it's the only place it's used, but perhaps not since I would have to move the corresponding class definition into the SectionMemoryManager as I did here

[justice_adams]

@lhames ping for update

[lhames]

Hi Justice,

Thanks for your patience with this review.

In D107087#2947188, @justice_adams wrote:

@lhames You are correct! I wasn't familiar with ManagedStatic but that does fix it. Do you prefer that approach to what's here? If so, I'll go ahead and update the diff.

Yes -- I think ManagedStatic should be preferred approach here.

• Lang.

[justice_adams]

@lhames update made. Thank you for the feedback/insight. If this looks good, could you go ahead and commit this for me (and possibly https://reviews.llvm.org/D107084 as well)

[lhames]

LGTM. Thanks Justice!

[lhames]

Thanks for this again Justice.

Looking again, I think we're arguably masking the issue here by leaking the default mapper (the tutorial doesn't call llvm_shutdown, which would free the managed statics). I think that's ok for now in a tutorial context, but I'll see if we can come up with a nicer long term solution too.

[lhames]

Looks like this broke some of the bots. E.g. https://lab.llvm.org/buildbot#builders/109/builds/22561.

A better solution to this problem might be to document that global SectionMemoryManagers are not allowed, then update the tutorial to avoid using them. I'll look in to that.

[justice_adams]

@lhames Sorry for the broken build, I didn't realize clang tests would be affected here.

I'm happy to continue working towards the correct approach here. Also thanks for the help so far!

A better solution to this problem might be to document that global SectionMemoryManagers are not allowed, then update the tutorial to avoid using them

Is the problem with SectionMemoryManager being global, or is it with the fact that the class internally relies on a variable of static storage duration (DefaultMMapperInstance) which may be wiped out before the SectionMemoryManagers own deconstruction?

If the solution is to enforce that no SectionMemoryManager are global, how would we enforce that programmatically?

In the tutorial case, the RTDyldObjectLinkingLayer holds onto the SectionMemoryManager instance, so it's not necessarily globally defined
https://reviews.llvm.org/source/llvm-github/browse/main/llvm/examples/Kaleidoscope/include/KaleidoscopeJIT.h$49
But the object it's contained in (The KaleidascopeJIT) is of static storage duration which causes the original error.

[lhames]

In D107087#3006722, @justice_adams wrote:

@lhames Sorry for the broken build, I didn't realize clang tests would be affected here.

Not a problem at all, I did not realize either. :)

This is what bots are for.

I'm happy to continue working towards the correct approach here. Also thanks for the help so far!

A better solution to this problem might be to document that global SectionMemoryManagers are not allowed, then update the tutorial to avoid using them

Is the problem with SectionMemoryManager being global, or is it with the fact that the class internally relies on a variable of static storage duration (DefaultMMapperInstance) which may be wiped out before the SectionMemoryManagers own deconstruction?

If the solution is to enforce that no SectionMemoryManager are global, how would we enforce that programmatically?

In the tutorial case, the RTDyldObjectLinkingLayer holds onto the SectionMemoryManager instance, so it's not necessarily globally defined
https://reviews.llvm.org/source/llvm-github/browse/main/llvm/examples/Kaleidoscope/include/KaleidoscopeJIT.h$49
But the object it's contained in (The KaleidascopeJIT) is of static storage duration which causes the original error.

Agreed. Enforcing this programmatically would be impossible. We've also seen this bug pop up in the wild since you filed this review, so I think it's likely to keep coming up even if we document that JIT's shouldn't be global.

I've reconsidered by position on this bug: SectionMemoryManager is a legacy API at this point, and it doesn't make sense to disallow global JIT objects just to keep the implementation of a legacy API clean and tidy.

Let's go with your original fix. Sorry for the run-around! :)

[justice_adams]

@lhames When you say "original fix" do you mean the fix where I moved DefaultMMapper to be a member of SectionMemoryManager?

I played around with the ManagedStatic approach some more, and see why the buildbots failed originally. It's because llvm_shutdown() is called by clang-repl.exe before the global JIT is destroyed when the program exits. Calling llvm_shutdown() makes sense, as this is the correct way to use the API.

But, if llvm_shutdown() is called, then when the program exits, the JIT is deconstructed (since it's global) and the SectionMemoryManager within the JIT is deconstructed as well, causing the original error to occur (a function called on a destroyed object since the DefaultMMapperInstance was already destroyed with llvm_shutdown())

Ultimately, we need to ensure that the MemoryMapper being held by the SectionMemoryManager is not destroyed prior to the destruction of the SectionMemoryManager instance. I came up with an approach using shared_ptrs which I think is the cleanest way I've found.

This ensures that
a.) if a MemoryMapper is allocated elsewhere and passed into the SectionMemoryManager constructor, then it can live on past the lifetime of the SectionMemoryManager instance due to the nature of shared_ptr's

and

b.) the SectionMemoryManager will instantiate a DefaultMMapper instance if needed, which will then deconstruct after the SectionMemoryManager is destroyed and the ref count is 0

This also allows us to keep DefaultMMapper where it's defined, which I agree makes the most sense.

I've updated the patch and ensured all LLVM + clang tests pass this time. Any thoughts?

[justice_adams]

use shared_ptrs

[justice_adams]

@lhames Ping for review

[lhames]

Sorry for the delayed reply.

@lhames When you say "original fix" do you mean the fix where I moved DefaultMMapper to be a member of SectionMemoryManager?

Yep. But I think anything that fixes this issue without unreasonable overhead is fine. As I said -- this is more-or-less legacy at this point.

The new approach looks ok too, but we don't allow SectionMemoryManagers to be copied so I think this could just as easily be a unique_ptr, couldn't it?

• Lang.

[RKSimon]

@justice_adams Is the anything blocking this from being committed?

[justice_adams]

@rgal no I don't think there is anything stopping this. Feel free to commit this for me (sorry for the delayed response)

[RKSimon]

@lhames Do you have any more feedback?

[justice_adams]

Ping for further feedback (or commit)

[sgraenitz]

This issues popped up with GCC 12 as well after https://github.com/llvm/llvm-project/commit/47f5c54f997a59bb2c65abe6b8b811f6e7553456

However, I don't think this patch is ready yet. I see two options:
(a) Keep the current approach to break the interface, fix all users of SectionMemoryManager (in MLIR for example, but I am not sure that's all) and maybe add a release note
(b) Distinguish between owned and non-owned MemoryMapper and fix only the DefaultMMapperInstance case that causes the Kaleidoscope issue

IMHO option (b) is the better solution, because it doesn't break the API and clients can get away without the shared_ptr. What do you think?

llvm/lib/ExecutionEngine/SectionMemoryManager.cpp

16

We don't need this anymore

[lhames]

@sgraenitz Option (b) sounds good to me.

[v.g.vassilev]

Hi all, any news on this patch?

[sgraenitz]

@justice_adams The patch linked above fixes the issue for GCC. Does it work for you as well? Also, there is a ticket here that looks very similar: https://github.com/llvm/llvm-project/issues/63204

[v.g.vassilev]

@sgraenitz, could we move forward with the patch as is? It fixes a whole bunch of issues for us.

[sgraenitz]

In D107087#4473634, @v.g.vassilev wrote:

@sgraenitz, could we move forward with the patch as is?

Well, it cannot land be cause it breaks the interface of SectionMemoryManager. If that gets fixed and the patch gets rebased, sure I will withdraw my veto.

It fixes a whole bunch of issues for us.

In terms of the original issue, however, my fix should cover everything from this patch. And given that there has been no progress in this review for 2 years, I am not sure this will move forward anymore. I guess it's the std::shared_ptr<MemoryMapper> that your are referring to? That's not a bug fix but rather a feature. I guess you can implement it independent from this review.

[sgraenitz]

This review should be closed

llvm/include/llvm/ExecutionEngine/SectionMemoryManager.h

public:
     virtual std::error_code releaseMappedMemory(sys::MemoryBlock &M) = 0;
 
     virtual ~MemoryMapper();
   };
 
   /// Creates a SectionMemoryManager instance with \p MM as the associated
   /// memory mapper.  If \p MM is nullptr then a default memory mapper is used
   /// that directly calls into the operating system.
-  SectionMemoryManager(MemoryMapper *MM = nullptr);
+  SectionMemoryManager(std::shared_ptr<MemoryMapper> = nullptr);
   SectionMemoryManager(const SectionMemoryManager &) = delete;
   void operator=(const SectionMemoryManager &) = delete;
   ~SectionMemoryManager() override;
 
   /// Allocates a memory block of (at least) the given size suitable for
   /// executable code.
   ///
   /// The value of \p Alignment must be a power of two.  If \p Alignment is zero
private:
   std::error_code applyMemoryGroupPermissions(MemoryGroup &MemGroup,
                                               unsigned Permissions);
 
   void anchor() override;
 
   MemoryGroup CodeMem;
   MemoryGroup RWDataMem;
   MemoryGroup RODataMem;
-  MemoryMapper &MMapper;
+  std::shared_ptr<MemoryMapper> MMapper;
 };
 
 } // end namespace llvm
 
 #endif // LLVM_EXECUTIONENGINE_SECTIONMEMORYMANAGER_H

llvm/lib/ExecutionEngine/SectionMemoryManager.cpp

 //===- SectionMemoryManager.cpp - Memory manager for MCJIT/RtDyld *- C++ -*-==//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the section-based memory manager used by the MCJIT
 // execution engine and RuntimeDyld
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ExecutionEngine/SectionMemoryManager.h"
 #include "llvm/Config/config.h"
+#include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/MathExtras.h"
 #include "llvm/Support/Process.h"
 
 namespace llvm {
 
 uint8_t *SectionMemoryManager::allocateDataSection(uintptr_t Size,
                                                    unsigned Alignment,
                                                    unsigned SectionID,
uint8_t *SectionMemoryManager::allocateSection(
   // be updated later based on memory group.
   //
   // FIXME: It would be useful to define a default allocation size (or add
   // it as a constructor parameter) to minimize the number of allocations.
   //
   // FIXME: Initialize the Near member for each memory group to avoid
   // interleaving.
   std::error_code ec;
-  sys::MemoryBlock MB = MMapper.allocateMappedMemory(
+  sys::MemoryBlock MB = MMapper->allocateMappedMemory(
       Purpose, RequiredSize, &MemGroup.Near,
       sys::Memory::MF_READ | sys::Memory::MF_WRITE, ec);
   if (ec) {
     // FIXME: Add error propagation to the interface.
     return nullptr;
   }
 
   // Save this address as the basis for our next request
static sys::MemoryBlock trimBlockToPageSize(sys::MemoryBlock M) {
 
   return Trimmed;
 }
 
 std::error_code
 SectionMemoryManager::applyMemoryGroupPermissions(MemoryGroup &MemGroup,
                                                   unsigned Permissions) {
   for (sys::MemoryBlock &MB : MemGroup.PendingMem)
-    if (std::error_code EC = MMapper.protectMappedMemory(MB, Permissions))
+    if (std::error_code EC = MMapper->protectMappedMemory(MB, Permissions))
       return EC;
 
   MemGroup.PendingMem.clear();
 
   // Now go through free blocks and trim any of them that don't span the entire
   // page because one of the pending blocks may have overlapped it.
   for (FreeMemBlock &FreeMB : MemGroup.FreeMem) {
     FreeMB.Free = trimBlockToPageSize(FreeMB.Free);
void SectionMemoryManager::invalidateInstructionCache() {
   for (sys::MemoryBlock &Block : CodeMem.PendingMem)
     sys::Memory::InvalidateInstructionCache(Block.base(),
                                             Block.allocatedSize());
 }
 
 SectionMemoryManager::~SectionMemoryManager() {
   for (MemoryGroup *Group : {&CodeMem, &RWDataMem, &RODataMem}) {
     for (sys::MemoryBlock &Block : Group->AllocatedMem)
-      MMapper.releaseMappedMemory(Block);
+      MMapper->releaseMappedMemory(Block);
   }
 }
 
 SectionMemoryManager::MemoryMapper::~MemoryMapper() {}
 
 void SectionMemoryManager::anchor() {}
 
 namespace {
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) {}
+SectionMemoryManager::SectionMemoryManager(std::shared_ptr<MemoryMapper> MM)
+    : MMapper(MM ? MM : std::make_shared<DefaultMMapper>()) {}
 
 } // namespace llvm