[Clang] Give error message for invalid profile path when compiling IR

[aidengrossman]

Summary

Before this patch, when compiling an IR file (eg the .llvmbc section
from an object file compiled with -Xclang -fembed-bitcode=all) and
profile data was passed in using the -fprofile-instrument-use-path
flag, there would be no error printed (as the previous implementation
relied on the error getting caught again in the constructor of
CodeGenModule which isn't called when -x ir is set). This patch
moves the error checking directly to where the error is caught
originally rather than failing silently in setPGOUseInstrumentor and
waiting to catch it in CodeGenModule to print diagnostic information to
the user.

Regression test added.

[aidengrossman]

Pinging reviewers based on previous commits in setPGOUseInstrumentor.

If there is an alternative method of writing this patch (eg placing the check in an equivalent of CodeGenModule for the IR side), let me know and I can adjust it. I'm not very familiar with the clang codebase, so there might be some inefficiencies in how I've implemented things.

[xur]

I'm fine with this change.

clang/lib/Frontend/CompilerInvocation.cpp

1301

Should we use "Error in reading profile ..."?
Reader still can return error even if the profile can be read.

[aidengrossman]

Changed error message from "Could not read profile" to "Error in reading profile"
to better reflect the fact that profile reading can fail for multiple reasons, not just
when the file path cannot be read/does not exist.

[aidengrossman]

Fix tests and replace error message in CodeGenModule with assertion since we're
now capturing the error message in CompileInvocation.

[aidengrossman]

@xur I've modified the patch slightly (mainly fixing tests and changing the error message printing in CodeGenModule to an assert as we should be capturing everything in CompilerInvocation. Do you mind looking over these changes, specifically making sure that going through CompilerInvocation is always guaranteed if we end up hitting CodeGenModule?

[mtrofin]

In D132991#3764877, @aidengrossman wrote:

@xur I've modified the patch slightly (mainly fixing tests and changing the error message printing in CodeGenModule to an assert as we should be capturing everything in CompilerInvocation. Do you mind looking over these changes, specifically making sure that going through CompilerInvocation is always guaranteed if we end up hitting CodeGenModule?

How come the old error checking in CodeGenModule wasn't tripped before?

[aidengrossman]

CodeGenModule doesn't get invoked when compiling IR. The error would trip when compiling other languages (eg c), but when passing IR to clang, the function doing error checking would never get called so no error was ever thrown.

[aidengrossman]

In regards to the review, I have checked most cases, and I'm pretty confident this patch should cover all cases (given that CompilerInvocation is used directly from clang's main()), but if there are any edge cases that somehow pop up due to this patch, it would be due to switching to an assert within CodeGenModule.

[aidengrossman]

@xur Gentle reminder. Just want to make sure I'm not missing anything obvious since I've made some somewhat substantial changes since your sign off.

clang/lib/CodeGen/CodeGenModule.cpp

CodeGenModule::CodeGenModule(ASTContext &C,
   Block.GlobalUniqueCount = 0;
 
   if (C.getLangOpts().ObjC)
     ObjCData.reset(new ObjCEntrypoints());
 
   if (CodeGenOpts.hasProfileClangUse()) {
     auto ReaderOrErr = llvm::IndexedInstrProfReader::create(
         CodeGenOpts.ProfileInstrumentUsePath, CodeGenOpts.ProfileRemappingFile);
-    if (auto E = ReaderOrErr.takeError()) {
-      unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,
-                                              "Could not read profile %0: %1");
-      llvm::handleAllErrors(std::move(E), [&](const llvm::ErrorInfoBase &EI) {
-        getDiags().Report(DiagID) << CodeGenOpts.ProfileInstrumentUsePath
-                                  << EI.message();
-      });
-    } else
-      PGOReader = std::move(ReaderOrErr.get());
+    // We're checking for profile read errors in CompilerInvocation, so if
+    // there was an error it should've already been caught. If it hasn't been
+    // somehow, trip an assertion.
+    assert(ReaderOrErr);
   }
 
   // If coverage mapping generation is enabled, create the
   // CoverageMappingModuleGen object.
   if (CodeGenOpts.CoverageMapping)
     CoverageMapping.reset(new CoverageMappingModuleGen(*this, *CoverageInfo));
 
   // Generate the module name hash here if needed.

clang/lib/Frontend/CompilerInvocation.cpp

static std::string serializeXRayInstrumentationBundle(const XRayInstrSet &S) {
   std::string Buffer;
   llvm::raw_string_ostream OS(Buffer);
   llvm::interleave(BundleParts, OS, [&OS](StringRef Part) { OS << Part; }, ",");
   return Buffer;
 }
 
 // Set the profile kind using fprofile-instrument-use-path.
 static void setPGOUseInstrumentor(CodeGenOptions &Opts,
-                                  const Twine &ProfileName) {
+                                  const Twine &ProfileName,
+                                  DiagnosticsEngine &Diags) {
   auto ReaderOrErr = llvm::IndexedInstrProfReader::create(ProfileName);
-  // In error, return silently and let Clang PGOUse report the error message.
   if (auto E = ReaderOrErr.takeError()) {
-    llvm::consumeError(std::move(E));
-    Opts.setProfileUse(CodeGenOptions::ProfileClangInstr);
+    unsigned DiagID = Diags.getCustomDiagID(DiagnosticsEngine::Error,
+                                            "Error in reading profile %0: %1");
+    llvm::handleAllErrors(std::move(E), [&](const llvm::ErrorInfoBase &EI) {
+      Diags.Report(DiagID) << ProfileName.str() << EI.message();
+    });
     return;
   }
   std::unique_ptr<llvm::IndexedInstrProfReader> PGOReader =
     std::move(ReaderOrErr.get());
   if (PGOReader->isIRLevelProfile()) {
     if (PGOReader->hasCSIRLevelProfile())
       Opts.setProfileUse(CodeGenOptions::ProfileCSIRInstr);
     else
if (Value != "simple" && Value != "mangled")
           << A->getOption().getName() << A->getValue();
     Opts.setDebugSimpleTemplateNames(
         StringRef(A->getValue()) == "simple"
             ? codegenoptions::DebugTemplateNamesKind::Simple
             : codegenoptions::DebugTemplateNamesKind::Mangled);
   }
 
   if (!Opts.ProfileInstrumentUsePath.empty())
-    setPGOUseInstrumentor(Opts, Opts.ProfileInstrumentUsePath);
+    setPGOUseInstrumentor(Opts, Opts.ProfileInstrumentUsePath, Diags);
 
   if (const Arg *A = Args.getLastArg(OPT_ftime_report, OPT_ftime_report_EQ)) {
     Opts.TimePasses = true;
 
     // -ftime-report= is only for new pass manager.
     if (A->getOption().getID() == OPT_ftime_report_EQ) {
       StringRef Val = A->getValue();
       if (Val == "per-pass")

clang/test/Profile/profile-does-not-exist-ir.c

+// RUN: not %clang_cc1 -emit-llvm -x ir %s -o - -fprofile-instrument-use-path=%t.nonexistent.profdata 2>&1 | FileCheck %s
+
+// CHECK: error: Error in reading profile {{.*}}.nonexistent.profdata:
+// CHECK-NOT: Assertion failed

clang/test/Profile/profile-does-not-exist.c

 // RUN: not %clang_cc1 -emit-llvm %s -o - -fprofile-instrument-use-path=%t.nonexistent.profdata 2>&1 | FileCheck %s
 
-// CHECK: error: Could not read profile {{.*}}.nonexistent.profdata:
+// CHECK: error: Error in reading profile {{.*}}.nonexistent.profdata:
 // CHECK-NOT: Assertion failed