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

[Arthur Eubanks via Phabricator via cfe-commits]

aeubanks added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:355-356
+      if (CodeGenOpts.ClearASTBeforeBackend) {
+        // The ASTContext may be unusable after this.
+        C.cleanup();
         C.getAllocator().Reset();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111767