Improve error message from FrontendAction

[jfb]

Summary

It's called in a few place, and for D63518 I initially set all their error propagation to ignore errors so we could propagate them one at a time. This change takes that second step and enables propagation, improving some module-related diagnostics.

[jfb]

There's a failure in clang/test/Index/pch-from-libclang.c. @thakis disabled that test for all but Darwin in r352809, and with my change I now get:

Internal compiler error: LFS error for "/Users/jfb/s/llvmm/debug/tools/clang/test/Index/Output/pch-from-libclang.c.tmp.mcp/14EC4PG1UTVLY/modules.idx"failed to create unique file /Users/jfb/s/llvmm/debug/tools/clang/test/Index/Output/pch-from-libclang.c.tmp.mcp/14EC4PG1UTVLY/modules.idx.lock-0ae18733: No such file or directory

From the test, it's the result of running:

/Users/jfb/s/llvmm/debug/bin/clang -fsyntax-only -include /Users/jfb/s/llvmm/debug/tools/clang/test/Index/Output/pch-from-libclang.c.tmp.h /Users/jfb/s/llvmm/clang/test/Index/pch-from-libclang.c -Xclang -verify -fmodules -fmodules-cache-path=/Users/jfb/s/llvmm/debug/tools/clang/test/Index/Output/pch-from-libclang.c.tmp.mcp -Xclang -detailed-preprocessing-record -Xclang -triple -Xclang x86_64-apple-darwin -Xclang -fallow-pch-with-compiler-errors

@akyrtzi do you know what's going on? I haven't investigate yet, and am hoping you'll just know :)

[akyrtzi]

Sorry, it's not clear to me what is the issue.

[jfb]

My current guess is that this part of the test:

c-index-test -write-pch %t.h.pch %s -fmodules -fmodules-cache-path=%t.mcp -Xclang -triple -Xclang x86_64-apple-darwin

Is expected to generate the unknown type name error, but when than happens it ignores -fmodules-cache-path=%t.mcp and doesn't create the directory. The next line expects the directory to exist, which is why it can't create the lock file (because the directory it's trying to create it in doesn't exist).

[akyrtzi]

In D65846#1619645, @jfb wrote:

My current guess is that this part of the test:

c-index-test -write-pch %t.h.pch %s -fmodules -fmodules-cache-path=%t.mcp -Xclang -triple -Xclang x86_64-apple-darwin

Is expected to generate the unknown type name error, but when than happens it ignores -fmodules-cache-path=%t.mcp and doesn't create the directory. The next line expects the directory to exist, which is why it can't create the lock file (because the directory it's trying to create it in doesn't exist).

clang -fmodules -fmodules-cache-path=... is supposed to create the directory for the cache path, including the parent directories, AFAIK. If this doesn't happen it is a behavior change (and undesirable IMO).

[bruno]

Internal compiler error: LFS error for "/Users/jfb/s/llvmm/debug/tools/clang/test/Index/Output/pch-from-libclang.c.tmp.mcp/14EC4PG1UTVLY/modules.idx"failed to create unique file /Users/jfb/s/llvmm/debug/tools/clang/test/Index/Output/pch-from-libclang.c.tmp.mcp/14EC4PG1UTVLY/modules.idx.lock-0ae18733: No such file or directory

yay! I really like how it looks now, thanks for pushing on this.

clang -fmodules -fmodules-cache-path=... is supposed to create the directory for the cache path, including the parent directories, AFAIK. If this doesn't happen it is a behavior change (and undesirable IMO).

Is c-index-test invoking clang or do we just have a similar interface? Perhaps it's not doing it right (haven't seen this problem happening directly while invoking clang). Should -fallow-pch-with-compiler-errors be considered somehow here?

[akyrtzi]

In D65846#1619752, @bruno wrote:

clang -fmodules -fmodules-cache-path=... is supposed to create the directory for the cache path, including the parent directories, AFAIK. If this doesn't happen it is a behavior change (and undesirable IMO).

Is c-index-test invoking clang or do we just have a similar interface? Perhaps it's not doing it right (haven't seen this problem happening directly while invoking clang). Should -fallow-pch-with-compiler-errors be considered somehow here?

To clarify, c-index-test only uses the libclang APIs. JF said "The next line expects the directory to exist", which I assume refers to the clang invocation, which is why I mention that a clang invocation does not need to expect for the cache directory to exist.
Also -fallow-pch-with-compiler-errors is always enabled by the libclang API that is used to create a PCH.

clang/lib/Frontend/CompilerInstance.cpp

bool CompilerInstance::ExecuteAction(FrontendAction &Act) {
 
   for (const FrontendInputFile &FIF : getFrontendOpts().Inputs) {
     // Reset the ID tables if we are reusing the SourceManager and parsing
     // regular files.
     if (hasSourceManager() && !Act.isModelParsingAction())
       getSourceManager().clearIDTables();
 
     if (Act.BeginSourceFile(*this, FIF)) {
-      if (llvm::Error Err = Act.Execute()) {
-        consumeError(std::move(Err)); // FIXME this drops errors on the floor.
-      }
+      if (llvm::Error Err = Act.Execute())
+        OS << "Internal compiler error: " << toString(std::move(Err)) << "\n";
       Act.EndSourceFile();
     }
   }
 
   // Notify the diagnostic client that all files were processed.
   getDiagnostics().getClient()->finish();
 
   if (getDiagnosticOpts().ShowCarets) {

clang/lib/Frontend/FrontendAction.cpp

llvm::Error FrontendAction::Execute() {
   else ExecuteAction();
 
   // If we are supposed to rebuild the global module index, do so now unless
   // there were any module-build failures.
   if (CI.shouldBuildGlobalModuleIndex() && CI.hasFileManager() &&
       CI.hasPreprocessor()) {
     StringRef Cache =
         CI.getPreprocessor().getHeaderSearchInfo().getModuleCachePath();
-    if (!Cache.empty()) {
+    if (!Cache.empty())
       if (llvm::Error Err = GlobalModuleIndex::writeIndex(
-              CI.getFileManager(), CI.getPCHContainerReader(), Cache)) {
-        // FIXME this drops the error on the floor, but
-        // Index/pch-from-libclang.c seems to rely on dropping at least some of
-        // the error conditions!
-        consumeError(std::move(Err));
-      }
-    }
+              CI.getFileManager(), CI.getPCHContainerReader(), Cache))
+        return Err;
   }
 
   return llvm::Error::success();
 }
 
 void FrontendAction::EndSourceFile() {
   CompilerInstance &CI = getCompilerInstance();
 

clang/lib/Serialization/GlobalModuleIndex.cpp

GlobalModuleIndex::writeIndex(FileManager &FileMgr,
   IndexPath += Path;
   llvm::sys::path::append(IndexPath, IndexFileName);
 
   // Coordinate building the global index file with other processes that might
   // try to do the same.
   llvm::LockFileManager Locked(IndexPath);
   switch (Locked) {
   case llvm::LockFileManager::LFS_Error:
-    return llvm::createStringError(std::errc::io_error, "LFS error");
+    return llvm::createStringError(
+        std::errc::io_error, "LFS error for \"%s\": %s", IndexPath.c_str(),
+        Locked.getErrorMessage().c_str());
 
   case llvm::LockFileManager::LFS_Owned:
     // We're responsible for building the index ourselves. Do so below.
     break;
 
   case llvm::LockFileManager::LFS_Shared:
     // Someone else is responsible for building the index. We don't care
     // when they finish, so we're done.
-    return llvm::createStringError(std::errc::device_or_resource_busy,
-                                   "someone else is building the index");
+    return llvm::createStringError(
+        std::errc::device_or_resource_busy,
+        "someone else is building the index \"%s\": %s", IndexPath.c_str(),
+        Locked.getErrorMessage().c_str());
   }
 
   // The module index builder.
   GlobalModuleIndexBuilder Builder(FileMgr, PCHContainerRdr);
 
   // Load each of the module files.
   std::error_code EC;
   for (llvm::sys::fs::directory_iterator D(Path, EC), DEnd;