[clang][Frontend] Add missing error handling

[LemonBoy]

Summary

Some early errors during the ASTUnit creation were not transferred to the FailedParseDiagnostic so when the code in LoadFromCommandLine swaps its content with the content of StoredDiagnostics they cannot be retrieved by the user in any way.

[LemonBoy]

Fix a C&P error in the attached test case.

[LemonBoy]

Ping?

[LemonBoy]

Ping?

[LemonBoy]

Ping?

[LemonBoy]

Ping?

[andrewrk]

ping?

[LemonBoy]

Ping with some more reviewers, hoping to land this in time for LLVM 99.

[dblaikie]

Perhaps this'd be more robust with ScopeExit? ( https://llvm.org/doxygen/ScopeExit_8h_source.html )

[LemonBoy]

Perhaps this'd be more robust with ScopeExit?

Not really, OnError is not executed when/if the function succeeds.

[dblaikie]

In D78658#2327615, @LemonBoy wrote:

Perhaps this'd be more robust with ScopeExit?

Not really, OnError is not executed when/if the function succeeds.

Ah, sorry - ScopeExit has a 'release()' function intended to disengage it/disable the execution of the payload.

The use would often look something like:

auto x = make_scope_exit(...);
if (failed())
  return fail;
if (failed())
  return fail;
x.release();
return success;

[LemonBoy]

Use ScopeExit instead of a bare lambda.

[dblaikie]

clang/lib/Frontend/ASTUnit.cpp

1158–1160

You can remove these braces now - since it's a superfluous change here now there's no need to add an extra function call.

1217–1218

Skip the braces here, since it's a single-line scope.

[dblaikie]

Looks good to me - thanks!

[LemonBoy]

Looks good to me - thanks!

Great! Can you please commit this for me as I have no commit access?

clang/lib/Frontend/ASTUnit.cpp

 #include "clang/Serialization/ModuleFile.h"
 #include "clang/Serialization/PCHContainerOperations.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/ADT/iterator_range.h"
 #include "llvm/Bitstream/BitstreamWriter.h"
if (OverrideMainBuffer) {
     Preamble->AddImplicitPreamble(*CCInvocation, VFS, OverrideMainBuffer.get());
     // VFS may have changed...
   }
 
   // Create the compiler instance to use for building the AST.
   std::unique_ptr<CompilerInstance> Clang(
       new CompilerInstance(std::move(PCHContainerOps)));
 
+  // Clean up on error, disengage it if the function returns successfully.
+  auto CleanOnError = llvm::make_scope_exit([&]() {
+    // Remove the overridden buffer we used for the preamble.
+    SavedMainFileBuffer = nullptr;
+
+    // Keep the ownership of the data in the ASTUnit because the client may
+    // want to see the diagnostics.
+    transferASTDataFromCompilerInstance(*Clang);
+    FailedParseDiagnostics.swap(StoredDiagnostics);
+    StoredDiagnostics.clear();
+    NumStoredDiagnosticsFromDriver = 0;
+  });
+
   // Ensure that Clang has a FileManager with the right VFS, which may have
   // changed above in AddImplicitPreamble.  If VFS is nullptr, rely on
   // createFileManager to create one.
   if (VFS && FileMgr && &FileMgr->getVirtualFileSystem() == VFS)
     Clang->setFileManager(&*FileMgr);
   else
     FileMgr = Clang->createFileManager(std::move(VFS));
 
   // Recover resources if we crash before exiting this method.
   llvm::CrashRecoveryContextCleanupRegistrar<CompilerInstance>
     CICleanup(Clang.get());
 
   Clang->setInvocation(CCInvocation);
   OriginalSourceFile =
       std::string(Clang->getFrontendOpts().Inputs[0].getFile());
 
   // Set up diagnostics, capturing any diagnostics that would
   // otherwise be dropped.
   Clang->setDiagnostics(&getDiagnostics());
 
   // Create the target instance.
   Clang->setTarget(TargetInfo::CreateTargetInfo(
       Clang->getDiagnostics(), Clang->getInvocation().TargetOpts));
   if (!Clang->hasTarget())
     return true;
 
   // Inform the target of the language options.
   //
   // FIXME: We shouldn't need to do this, the target should be immutable once
   // created. This complexity should be lifted elsewhere.
   Clang->getTarget().adjust(Clang->getLangOpts());
 
   assert(Clang->getFrontendOpts().Inputs.size() == 1 &&
          "Invocation must have exactly one source file!");
bool ASTUnit::Parse(std::shared_ptr<PCHContainerOperations> PCHContainerOps,
   std::unique_ptr<TopLevelDeclTrackerAction> Act(
       new TopLevelDeclTrackerAction(*this));
 
   // Recover resources if we crash before exiting this method.
   llvm::CrashRecoveryContextCleanupRegistrar<TopLevelDeclTrackerAction>
     ActCleanup(Act.get());
 
   if (!Act->BeginSourceFile(*Clang.get(), Clang->getFrontendOpts().Inputs[0]))
-    goto error;
+    return true;
 
   if (SavedMainFileBuffer)
     TranslateStoredDiagnostics(getFileManager(), getSourceManager(),
                                PreambleDiagnostics, StoredDiagnostics);
   else
     PreambleSrcLocCache.clear();
 
   if (llvm::Error Err = Act->Execute()) {
     consumeError(std::move(Err)); // FIXME this drops errors on the floor.
-    goto error;
+    return true;
   }
 
   transferASTDataFromCompilerInstance(*Clang);
 
   Act->EndSourceFile();
 
   FailedParseDiagnostics.clear();
 
-  return false;
-
-error:
-  // Remove the overridden buffer we used for the preamble.
-  SavedMainFileBuffer = nullptr;
+  CleanOnError.release();
 
-  // Keep the ownership of the data in the ASTUnit because the client may
-  // want to see the diagnostics.
-  transferASTDataFromCompilerInstance(*Clang);
-  FailedParseDiagnostics.swap(StoredDiagnostics);
-  StoredDiagnostics.clear();
-  NumStoredDiagnosticsFromDriver = 0;
-  return true;
+  return false;
 }
 
 static std::pair<unsigned, unsigned>
 makeStandaloneRange(CharSourceRange Range, const SourceManager &SM,
                     const LangOptions &LangOpts) {
   CharSourceRange FileRange = Lexer::makeFileCharRange(Range, SM, LangOpts);
   unsigned Offset = SM.getFileOffset(FileRange.getBegin());
   unsigned EndOffset = SM.getFileOffset(FileRange.getEnd());

clang/unittests/Frontend/ASTUnitTest.cpp

TEST_F(ASTUnitTest, ModuleTextualHeader) {
   ASSERT_TRUE(AU);
   auto File = AU->getFileManager().getFileRef("Textual.h", false, false);
   ASSERT_TRUE(bool(File));
   // Verify that we do not crash here.
   EXPECT_TRUE(AU->getPreprocessor().getHeaderSearchInfo().getExistingFileInfo(
       &File->getFileEntry()));
 }
 
+TEST_F(ASTUnitTest, LoadFromCommandLineEarlyError) {
+  EXPECT_FALSE(
+      llvm::sys::fs::createTemporaryFile("ast-unit", "c", FD, InputFileName));
+  input_file = std::make_unique<ToolOutputFile>(InputFileName, FD);
+  input_file->os() << "";
+
+  const char *Args[] = {"clang", "-target", "foobar", InputFileName.c_str()};
+
+  auto Diags = CompilerInstance::createDiagnostics(new DiagnosticOptions());
+  auto PCHContainerOps = std::make_shared<PCHContainerOperations>();
+  std::unique_ptr<clang::ASTUnit> ErrUnit;
+
+  ASTUnit *AST = ASTUnit::LoadFromCommandLine(
+      &Args[0], &Args[4], PCHContainerOps, Diags, "", false,
+      CaptureDiagsKind::All, None, true, 0, TU_Complete, false, false, false,
+      SkipFunctionBodiesScope::None, false, true, false, false, None, &ErrUnit,
+      nullptr);
+
+  ASSERT_EQ(AST, nullptr);
+  ASSERT_NE(ErrUnit, nullptr);
+  ASSERT_TRUE(Diags->hasErrorOccurred());
+  ASSERT_NE(ErrUnit->stored_diag_size(), 0U);
+}
+
 } // anonymous namespace