[PATCH] D111767: [clang] Support -clear-ast-before-backend without -disable-free

[David Blaikie via Phabricator via cfe-commits]

dblaikie accepted this revision.
dblaikie added subscribers: aaron.ballman, rsmith.
dblaikie added a comment.
This revision is now accepted and ready to land.

Sounds OK, thanks!



================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:355-356
+      if (CodeGenOpts.ClearASTBeforeBackend) {
+        // The ASTContext may be unusable after this.
+        C.cleanup();
         C.getAllocator().Reset();
----------------
aeubanks wrote:
> dblaikie wrote:
> > Any chance of refactoring such that the ASTContext is scoped such that it is destroyed here, rather than rendered unusable - to reduce the chance that it'd be accidentally used after this point?
> > 
> > Like what happens if CompilerInstance::setASTContext(nullptr) is called? It wouldn't null out everyone's ASTContext pointers, but would mean there's no object there - perhaps crash harder/faster than a "cleaned up" ASTContext being used?
> I had tried this in the past and gave up for some reason. So I tried again (ASan was very helpful here) and ran into a couple of issues.
> 
> First, Sema depends on ASTContext, so we have to `setSema(nullptr)` before `setASTContext(nullptr)`, but Sema has to finalize some things after codegen, so we can't `CI->setSema(nullptr)` until much later.
> 
> Another unrelated thing is that the diagnostics end up calling into ASTContext. This might be fixable by moving some data structures out of ASTContext?
> 
> Another unrelated thing is a weird crash in `DiagStorageAllocator::~DiagStorageAllocator()` when calling `setASTContext(nullptr)` early.
> 
> These individually might be solvable, but it seems like a lot of unrelated issues.
> Basically, we depend a lot on various parts of ASTContext that aren't the actual AST nodes themselves.
Fair enough - not sure, but might be worth noting this info somewhere in the source in case someone else comes along to do this refactoring in the future if they have another reason to weigh into the choice to address it.

@rsmith @aaron.ballman - wouldn't mind at least a quick thought on this stuff if you've both got a moment - for long term direction/broad design of this stuff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111767