[PATCH] D107087: Fix SectionMemoryManager deconstruction error with MSVC

Mon Sep 27 13:07:57 PDT 2021

lhames added a comment.

In D107087#3006722 <https://reviews.llvm.org/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 `SectionMemoryManager`s 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 `SectionMemoryManager`s 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! :)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107087