Skip to content
Snippets Groups Projects
Commit fa5f2ec7 authored by Duncan P. N. Exon Smith's avatar Duncan P. N. Exon Smith
Browse files

Reapply "Frontend: Stop leaking when not -disable-free"

This reverts commit r236422, effectively reapplying r236419.  ASan
helped me diagnose the problem: the non-leaking logic would free the
ASTConsumer before freeing Sema whenever `isCurrentASTFile()`, causing a
use-after-free in `Sema::~Sema()`.

This version unconditionally frees Sema and the ASTContext before
freeing the ASTConsumer.  Without the fix, these were either being freed
before the ASTConsumer was freed or leaked after, but they were always
spiritually released so this isn't really a functionality change.

I ran all of check-clang with ASan locally this time, so I'm hoping
there aren't any more problems lurking.

Original commit message:

    Try again to plug a leak that's been around since at least r128011
    after coming across the FIXME.  Nico Weber tried something similar
    in r207065 but had to revert in r207070 due to a bot failure.

    The build failure isn't visible anymore so I'm not sure what went
    wrong.  I'm doing this slightly differently -- when not
    -disable-free I'm still resetting the members (just not leaking
    them) -- so maybe it will work out this time?  Tests pass locally,
    anyway.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@236424 91177308-0d34-0410-b5e6-96231b3b80d8
parent 38ddb0ae
No related branches found
No related tags found
No related merge requests found
...@@ -468,16 +468,12 @@ void FrontendAction::EndSourceFile() { ...@@ -468,16 +468,12 @@ void FrontendAction::EndSourceFile() {
// FIXME: There is more per-file stuff we could just drop here? // FIXME: There is more per-file stuff we could just drop here?
bool DisableFree = CI.getFrontendOpts().DisableFree; bool DisableFree = CI.getFrontendOpts().DisableFree;
if (DisableFree) { if (DisableFree) {
if (!isCurrentFileAST()) { CI.resetAndLeakSema();
CI.resetAndLeakSema(); CI.resetAndLeakASTContext();
CI.resetAndLeakASTContext();
}
BuryPointer(CI.takeASTConsumer().get()); BuryPointer(CI.takeASTConsumer().get());
} else { } else {
if (!isCurrentFileAST()) { CI.setSema(nullptr);
CI.setSema(nullptr); CI.setASTContext(nullptr);
CI.setASTContext(nullptr);
}
CI.setASTConsumer(nullptr); CI.setASTConsumer(nullptr);
} }
...@@ -494,13 +490,16 @@ void FrontendAction::EndSourceFile() { ...@@ -494,13 +490,16 @@ void FrontendAction::EndSourceFile() {
// FrontendAction. // FrontendAction.
CI.clearOutputFiles(/*EraseFiles=*/shouldEraseOutputFiles()); CI.clearOutputFiles(/*EraseFiles=*/shouldEraseOutputFiles());
// FIXME: Only do this if DisableFree is set.
if (isCurrentFileAST()) { if (isCurrentFileAST()) {
CI.resetAndLeakSema(); if (DisableFree) {
CI.resetAndLeakASTContext(); CI.resetAndLeakPreprocessor();
CI.resetAndLeakPreprocessor(); CI.resetAndLeakSourceManager();
CI.resetAndLeakSourceManager(); CI.resetAndLeakFileManager();
CI.resetAndLeakFileManager(); } else {
CI.setPreprocessor(nullptr);
CI.setSourceManager(nullptr);
CI.setFileManager(nullptr);
}
} }
setCompilerInstance(nullptr); setCompilerInstance(nullptr);
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment