Page MenuHomePhabricator

Adding doWithCleanup abstraction
Needs ReviewPublic

Authored by beanz on Wed, Sep 22, 12:01 PM.

Details

Reviewers
lhames
dblaikie
Summary

This patch adds a new doWithCleanup abstraction to LLVM that works
with llvm::Error, int, bool, and <insert your own type here>
return types that propagate status.

This abstraction is a more general way of having a conditional cleanup
that can be used with multiple return paths out of a function.

Test cases and a first use case in clang are part of the patch.

Diff Detail

Unit TestsFailed

TimeTest
120 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-cxa-atexit.S
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-cxa-atexit.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-cxa-atexit.S
100 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-static-initializer.S
Script: -- : 'RUN: at line 7'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-static-initializer.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-static-initializer.S
100 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-tls.S
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-tls.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-tls.S

Event Timeline

beanz created this revision.Wed, Sep 22, 12:01 PM
beanz requested review of this revision.Wed, Sep 22, 12:01 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptWed, Sep 22, 12:01 PM
dblaikie added inline comments.Thu, Sep 30, 8:52 PM
clang/lib/Frontend/FrontendAction.cpp
553–565

I guess this would be written as:

bool Success = BeginSourceFileImpl(CI, RealInput, HasBegunSourceFile))
if (!Success) {
  if (HasBegunSourceFile)
    CI.getDiagnosticClient().EndSourceFile();
  CI.clearOutputFiles(/*EraseFiles=*/true);
  CI.getLangOpts().setCompilingModule(LangOptions::CMK_None);
  setCurrentInput(FrontendInputFile());
  setCompilerInstance(nullptr);
}
return Success;

Which seems pretty legible - makes me unsure it's worth the complexity of building something like doWithCleanup, not that it's heinously complicated by any means - but all the traits stuff, etc.

& ideally/in general this'd usually be dealt with with some kind of RAII ownership model rather than such a strong stateful transition of an existing object that needs to be undone, rather than destroying an object.

llvm/include/llvm/Support/Cleanup.h
56–59

If this is going to be a template anyway, might as well use raw/generic/templated functor arguments and avoid the dynamic dispatch overhead of std::function? (or at least use llvm::function_ref)

llvm/unittests/Support/CleanupTests.cpp
19

Presumably doWithCleanup doesn't need to be called with an explicit template type argument most of the time/ever? It's probably good to test it in that way too, without passing the explicit template type argument.