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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
