[PATCH] D107087: Fix SectionMemoryManager deconstruction error with MSVC

[Justice Adams via Phabricator via llvm-commits]

justice_adams added a comment.

@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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107087/new/

https://reviews.llvm.org/D107087