[NFC] `goto fail` has failed us in the past...

[beanz]

Summary

This patch replaces reliance on goto failure pattern with
llvm::scope_exit.

[bkramer]

Looks good, thanks

[dblaikie]

(A possibly more robust option - so that this doesn't regress if someone adds a new "return true" codepath & forgets to release the cleanup (it looks like a pretty long function with several exits, etc) - would be to move the code into an "impl" function and have the original function call it, check the result, and do the cleanup once?)

[beanz]

I was talking with @lhames the other day about building a doWithCleanup abstraction that could take a labmda to perform and a lambda to cleanup on failure.

I was thinking I may take a stab at that this week.

[dblaikie]

In D109865#3014106, @beanz wrote:

I was talking with @lhames the other day about building a doWithCleanup abstraction that could take a labmda to perform and a lambda to cleanup on failure.

I was thinking I may take a stab at that this week.

Sure, would be nice - though I'd probably still argue that a 400 line lambda might be a bit unwieldy/unclear.

clang/lib/Frontend/FrontendAction.cpp

 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/LiteralSupport.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Parse/ParseAST.h"
 #include "clang/Serialization/ASTDeserializationListener.h"
 #include "clang/Serialization/ASTReader.h"
 #include "clang/Serialization/GlobalModuleIndex.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/BuryPointer.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Timer.h"
 #include "llvm/Support/raw_ostream.h"
 #include <system_error>
 using namespace clang;
bool FrontendAction::BeginSourceFile(CompilerInstance &CI,
   assert(!Instance && "Already processing a source file!");
   assert(!Input.isEmpty() && "Unexpected empty filename!");
   setCurrentInput(Input);
   setCompilerInstance(&CI);
 
   bool HasBegunSourceFile = false;
   bool ReplayASTFile = Input.getKind().getFormat() == InputKind::Precompiled &&
                        usesPreprocessorOnly();
+
+  // If we fail, reset state since the client will not end up calling the
+  // matching EndSourceFile(). All paths that return true should release this.
+  auto FailureCleanup = llvm::make_scope_exit([&]() {
+    if (HasBegunSourceFile)
+      CI.getDiagnosticClient().EndSourceFile();
+    CI.clearOutputFiles(/*EraseFiles=*/true);
+    CI.getLangOpts().setCompilingModule(LangOptions::CMK_None);
+    setCurrentInput(FrontendInputFile());
+    setCompilerInstance(nullptr);
+  });
+
   if (!BeginInvocation(CI))
-    goto failure;
+    return false;
 
   // If we're replaying the build of an AST file, import it and set up
   // the initial state from its build.
   if (ReplayASTFile) {
     IntrusiveRefCntPtr<DiagnosticsEngine> Diags(&CI.getDiagnostics());
 
     // The AST unit populates its own diagnostics engine rather than ours.
     IntrusiveRefCntPtr<DiagnosticsEngine> ASTDiags(
         new DiagnosticsEngine(Diags->getDiagnosticIDs(),
                               &Diags->getDiagnosticOptions()));
     ASTDiags->setClient(Diags->getClient(), /*OwnsClient*/false);
 
     // FIXME: What if the input is a memory buffer?
     StringRef InputFile = Input.getFile();
 
     std::unique_ptr<ASTUnit> AST = ASTUnit::LoadFromASTFile(
         std::string(InputFile), CI.getPCHContainerReader(),
         ASTUnit::LoadPreprocessorOnly, ASTDiags, CI.getFileSystemOpts(),
         CI.getCodeGenOpts().DebugTypeExtRefs);
     if (!AST)
-      goto failure;
+      return false;
 
     // Options relating to how we treat the input (but not what we do with it)
     // are inherited from the AST unit.
     CI.getHeaderSearchOpts() = AST->getHeaderSearchOpts();
     CI.getPreprocessorOpts() = AST->getPreprocessorOpts();
     CI.getLangOpts() = AST->getLangOpts();
 
     // Set the shared objects, these are reset when we finish processing the
if (Input.getKind().getFormat() == InputKind::Precompiled) {
     StringRef InputFile = Input.getFile();
 
     std::unique_ptr<ASTUnit> AST = ASTUnit::LoadFromASTFile(
         std::string(InputFile), CI.getPCHContainerReader(),
         ASTUnit::LoadEverything, Diags, CI.getFileSystemOpts(),
         CI.getCodeGenOpts().DebugTypeExtRefs);
 
     if (!AST)
-      goto failure;
+      return false;
 
     // Inform the diagnostic client we are processing a source file.
     CI.getDiagnosticClient().BeginSourceFile(CI.getLangOpts(), nullptr);
     HasBegunSourceFile = true;
 
     // Set the shared objects, these are reset when we finish processing the
     // file, otherwise the CompilerInstance will happily destroy them.
     CI.setFileManager(&AST->getFileManager());
     CI.setSourceManager(&AST->getSourceManager());
     CI.setPreprocessor(AST->getPreprocessorPtr());
     Preprocessor &PP = CI.getPreprocessor();
     PP.getBuiltinInfo().initializeBuiltins(PP.getIdentifierTable(),
                                            PP.getLangOpts());
     CI.setASTContext(&AST->getASTContext());
 
     setCurrentInput(Input, std::move(AST));
 
     // Initialize the action.
     if (!BeginSourceFileAction(CI))
-      goto failure;
+      return false;
 
     // Create the AST consumer.
     CI.setASTConsumer(CreateWrappedASTConsumer(CI, InputFile));
     if (!CI.hasASTConsumer())
-      goto failure;
+      return false;
 
+    FailureCleanup.release();
     return true;
   }
 
   // Set up the file and source managers, if needed.
   if (!CI.hasFileManager()) {
     if (!CI.createFileManager()) {
-      goto failure;
+      return false;
     }
   }
   if (!CI.hasSourceManager())
     CI.createSourceManager(CI.getFileManager());
 
   // Set up embedding for any specified files. Do this before we load any
   // source files, including the primary module map for the compilation.
   for (const auto &F : CI.getFrontendOpts().ModulesEmbedFiles) {
assert(hasIRSupport() &&
            "This action does not have IR file support!");
 
     // Inform the diagnostic client we are processing a source file.
     CI.getDiagnosticClient().BeginSourceFile(CI.getLangOpts(), nullptr);
     HasBegunSourceFile = true;
 
     // Initialize the action.
     if (!BeginSourceFileAction(CI))
-      goto failure;
+      return false;
 
     // Initialize the main file entry.
     if (!CI.InitializeSourceManager(CurrentInput))
-      goto failure;
+      return false;
 
+    FailureCleanup.release();
     return true;
   }
 
   // If the implicit PCH include is actually a directory, rather than
   // a single file, search for a suitable PCH file in that directory.
   if (!CI.getPreprocessorOpts().ImplicitPCHInclude.empty()) {
     FileManager &FileMgr = CI.getFileManager();
     PreprocessorOptions &PPOpts = CI.getPreprocessorOpts();
if (auto PCHDir = FileMgr.getDirectory(PCHInclude)) {
           PPOpts.ImplicitPCHInclude = std::string(Dir->path());
           Found = true;
           break;
         }
       }
 
       if (!Found) {
         CI.getDiagnostics().Report(diag::err_fe_no_pch_in_dir) << PCHInclude;
-        goto failure;
+        return false;
       }
     }
   }
 
   // Set up the preprocessor if needed. When parsing model files the
   // preprocessor of the original source is reused.
   if (!isModelParsingAction())
     CI.createPreprocessor(getTranslationUnitKind());
 
   // Inform the diagnostic client we are processing a source file.
   CI.getDiagnosticClient().BeginSourceFile(CI.getLangOpts(),
                                            &CI.getPreprocessor());
   HasBegunSourceFile = true;
 
   // Initialize the main file entry.
   if (!CI.InitializeSourceManager(Input))
-    goto failure;
+    return false;
 
   // For module map files, we first parse the module map and synthesize a
   // "<module-includes>" buffer before more conventional processing.
   if (Input.getKind().getFormat() == InputKind::ModuleMap) {
     CI.getLangOpts().setCompilingModule(LangOptions::CMK_ModuleMap);
 
     std::string PresumedModuleMapFile;
     unsigned OffsetToContents;
     if (loadModuleMapForModuleBuild(CI, Input.isSystem(),
                                     Input.isPreprocessed(),
                                     PresumedModuleMapFile, OffsetToContents))
-      goto failure;
+      return false;
 
     auto *CurrentModule = prepareToBuildModule(CI, Input.getFile());
     if (!CurrentModule)
-      goto failure;
+      return false;
 
     CurrentModule->PresumedModuleMapFile = PresumedModuleMapFile;
 
     if (OffsetToContents)
       // If the module contents are in the same file, skip to them.
       CI.getPreprocessor().setSkipMainFilePreamble(OffsetToContents, true);
     else {
       // Otherwise, convert the module description to a suitable input buffer.
       auto Buffer = getInputBufferForModule(CI, CurrentModule);
       if (!Buffer)
-        goto failure;
+        return false;
 
       // Reinitialize the main file entry to refer to the new input.
       auto Kind = CurrentModule->IsSystem ? SrcMgr::C_System : SrcMgr::C_User;
       auto &SourceMgr = CI.getSourceManager();
       auto BufferID = SourceMgr.createFileID(std::move(Buffer), Kind);
       assert(BufferID.isValid() && "couldn't create module buffer ID");
       SourceMgr.setMainFileID(BufferID);
     }
   }
 
   // Initialize the action.
   if (!BeginSourceFileAction(CI))
-    goto failure;
+    return false;
 
   // If we were asked to load any module map files, do so now.
   for (const auto &Filename : CI.getFrontendOpts().ModuleMapFiles) {
     if (auto File = CI.getFileManager().getFile(Filename))
       CI.getPreprocessor().getHeaderSearchInfo().loadModuleMapFile(
           *File, /*IsSystem*/false);
     else
       CI.getDiagnostics().Report(diag::err_module_map_not_found) << Filename;
if (!usesPreprocessorOnly()) {
     // source file name with a linemarker.
     std::string PresumedInputFile = std::string(getCurrentFileOrBufferName());
     if (Input.isPreprocessed())
       ReadOriginalFileName(CI, PresumedInputFile);
 
     std::unique_ptr<ASTConsumer> Consumer =
         CreateWrappedASTConsumer(CI, PresumedInputFile);
     if (!Consumer)
-      goto failure;
+      return false;
 
     // FIXME: should not overwrite ASTMutationListener when parsing model files?
     if (!isModelParsingAction())
       CI.getASTContext().setASTMutationListener(Consumer->GetASTMutationListener());
 
     if (!CI.getPreprocessorOpts().ChainedIncludes.empty()) {
       // Convert headers to PCH and chain them.
       IntrusiveRefCntPtr<ExternalSemaSource> source, FinalReader;
       source = createChainedIncludesSource(CI, FinalReader);
       if (!source)
-        goto failure;
+        return false;
       CI.setASTReader(static_cast<ASTReader *>(FinalReader.get()));
       CI.getASTContext().setExternalSource(source);
     } else if (CI.getLangOpts().Modules ||
                !CI.getPreprocessorOpts().ImplicitPCHInclude.empty()) {
       // Use PCM or PCH.
       assert(hasPCHSupport() && "This action does not have PCH support!");
       ASTDeserializationListener *DeserialListener =
           Consumer->GetASTDeserializationListener();
if (!CI.getPreprocessorOpts().ChainedIncludes.empty()) {
       }
       if (!CI.getPreprocessorOpts().ImplicitPCHInclude.empty()) {
         CI.createPCHExternalASTSource(
             CI.getPreprocessorOpts().ImplicitPCHInclude,
             CI.getPreprocessorOpts().DisablePCHOrModuleValidation,
             CI.getPreprocessorOpts().AllowPCHWithCompilerErrors,
             DeserialListener, DeleteDeserialListener);
         if (!CI.getASTContext().getExternalSource())
-          goto failure;
+          return false;
       }
       // If modules are enabled, create the AST reader before creating
       // any builtins, so that all declarations know that they might be
       // extended by an external source.
       if (CI.getLangOpts().Modules || !CI.hasASTContext() ||
           !CI.getASTContext().getExternalSource()) {
         CI.createASTReader();
         CI.getASTReader()->setDeserializationListener(DeserialListener,
                                                       DeleteDeserialListener);
       }
     }
 
     CI.setASTConsumer(std::move(Consumer));
     if (!CI.hasASTConsumer())
-      goto failure;
+      return false;
   }
 
   // Initialize built-in info as long as we aren't using an external AST
   // source.
   if (CI.getLangOpts().Modules || !CI.hasASTContext() ||
       !CI.getASTContext().getExternalSource()) {
     Preprocessor &PP = CI.getPreprocessor();
     PP.getBuiltinInfo().initializeBuiltins(PP.getIdentifierTable(),
                                            PP.getLangOpts());
   } else {
     // FIXME: If this is a problem, recover from it by creating a multiplex
     // source.
     assert((!CI.getLangOpts().Modules || CI.getASTReader()) &&
            "modules enabled but created an external source that "
            "doesn't support modules");
   }
 
   // If we were asked to load any module files, do so now.
   for (const auto &ModuleFile : CI.getFrontendOpts().ModuleFiles)
     if (!CI.loadModuleFile(ModuleFile))
-      goto failure;
+      return false;
 
   // If there is a layout overrides file, attach an external AST source that
   // provides the layouts from that file.
   if (!CI.getFrontendOpts().OverrideRecordLayoutsFile.empty() &&
       CI.hasASTContext() && !CI.getASTContext().getExternalSource()) {
     IntrusiveRefCntPtr<ExternalASTSource>
       Override(new LayoutOverrideSource(
                      CI.getFrontendOpts().OverrideRecordLayoutsFile));
     CI.getASTContext().setExternalSource(Override);
   }
 
+  FailureCleanup.release();
   return true;
-
-  // If we failed, reset state since the client will not end up calling the
-  // matching EndSourceFile().
-failure:
-  if (HasBegunSourceFile)
-    CI.getDiagnosticClient().EndSourceFile();
-  CI.clearOutputFiles(/*EraseFiles=*/true);
-  CI.getLangOpts().setCompilingModule(LangOptions::CMK_None);
-  setCurrentInput(FrontendInputFile());
-  setCompilerInstance(nullptr);
-  return false;
 }
 
 llvm::Error FrontendAction::Execute() {
   CompilerInstance &CI = getCompilerInstance();
 
   if (CI.hasFrontendTimer()) {
     llvm::TimeRegion Timer(CI.getFrontendTimer());
     ExecuteAction();