[Clang] Fix PPChainedCallbacks::EmbedFileNotFound()

[cofibrant]

We've had internal test failures since #166188 landed. The root cause is that PPChainedCallbacks::EmbedFileNotFound() incorrectly calls PPCallbacks::FileNotFound() not PPCallbacks::EmbedFileNotFound().

[juliannagele]

LGTM, thanks!

[Fznamznon]

Thanks for the fix! Is that possible to add a test?

[cofibrant]

Thanks for the fix! Is that possible to add a test?

I'm not at all familiar with this part of the code so didn't know where to start w.r.t. testing. Do you have any suggestions?

[cor3ntin]

Oups, i missed this does not have a test.

a good way to start is to reduce the problem you are seeing - and then look at tests in clang/test/Preprocessor

[cofibrant]

The issue is only reproducible in when the lexer has chained preprocessor callbacks for the following test case: ‎clang/test/Driver/mg.c. Could either of you suggest another flag to add to the run line to add another set of preprocessor callbacks? (Although, I would count the original code as a typo.)

[llvmbot]

@llvm/pr-subscribers-clang

Author: Nathan Corbyn (cofibrant)

Changes

We've had internal test failures since #166188 landed. The root cause is that PPChainedCallbacks::EmbedFileNotFound() incorrectly calls PPCallbacks::FileNotFound() not PPCallbacks::EmbedFileNotFound().

---

Full diff: https://github.com/llvm/llvm-project/pull/170293.diff

3 Files Affected:

• (modified) clang/include/clang/Lex/PPCallbacks.h (+2-2)
• (modified) clang/lib/Lex/PPDirectives.cpp (+6-5)
• (modified) clang/unittests/Lex/PPCallbacksTest.cpp (+34)

diff --git a/clang/include/clang/Lex/PPCallbacks.h b/clang/include/clang/Lex/PPCallbacks.h
index 313b730afbab8..e6120c5648798 100644
--- a/clang/include/clang/Lex/PPCallbacks.h
+++ b/clang/include/clang/Lex/PPCallbacks.h
@@ -499,10 +499,10 @@ class PPChainedCallbacks : public PPCallbacks {
   }
 
   bool EmbedFileNotFound(StringRef FileName) override {
-    bool Skip = First->FileNotFound(FileName);
+    bool Skip = First->EmbedFileNotFound(FileName);
     // Make sure to invoke the second callback, no matter if the first already
     // returned true to skip the file.
-    Skip |= Second->FileNotFound(FileName);
+    Skip |= Second->EmbedFileNotFound(FileName);
     return Skip;
   }
 
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 891c8ab7f3155..1c3d9480651da 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -1397,11 +1397,12 @@ void Preprocessor::HandleDirective(Token &Result) {
       return HandleIdentSCCSDirective(Result);
     case tok::pp_sccs:
       return HandleIdentSCCSDirective(Result);
-    case tok::pp_embed:
-      return HandleEmbedDirective(SavedHash.getLocation(), Result,
-                                  getCurrentFileLexer()
-                                      ? *getCurrentFileLexer()->getFileEntry()
-                                      : static_cast<FileEntry *>(nullptr));
+    case tok::pp_embed: {
+      if (auto *CurrentFileLexer = getCurrentFileLexer())
+        if (auto FERef = CurrentFileLexer->getFileEntry())
+          return HandleEmbedDirective(SavedHash.getLocation(), Result, *FERef);
+      return HandleEmbedDirective(SavedHash.getLocation(), Result, nullptr);
+    }
     case tok::pp_assert:
       //isExtension = true;  // FIXME: implement #assert
       break;
diff --git a/clang/unittests/Lex/PPCallbacksTest.cpp b/clang/unittests/Lex/PPCallbacksTest.cpp
index 990689c6b1e45..f32fe5f6342be 100644
--- a/clang/unittests/Lex/PPCallbacksTest.cpp
+++ b/clang/unittests/Lex/PPCallbacksTest.cpp
@@ -463,6 +463,40 @@ TEST_F(PPCallbacksTest, FileNotFoundSkipped) {
   ASSERT_EQ(0u, DiagConsumer->getNumErrors());
 }
 
+TEST_F(PPCallbacksTest, EmbedFileNotFoundChained) {
+  const char *SourceText = "#embed \"notfound.h\"\n";
+
+  std::unique_ptr<llvm::MemoryBuffer> SourceBuf =
+      llvm::MemoryBuffer::getMemBuffer(SourceText);
+  SourceMgr.setMainFileID(SourceMgr.createFileID(std::move(SourceBuf)));
+
+  HeaderSearchOptions HSOpts;
+  TrivialModuleLoader ModLoader;
+  PreprocessorOptions PPOpts;
+  HeaderSearch HeaderInfo(HSOpts, SourceMgr, Diags, LangOpts, Target.get());
+
+  DiagnosticConsumer *DiagConsumer = new DiagnosticConsumer;
+  DiagnosticsEngine EmbedFileNotFoundDiags(DiagID, DiagOpts, DiagConsumer);
+  Preprocessor PP(PPOpts, EmbedFileNotFoundDiags, LangOpts, SourceMgr,
+                  HeaderInfo, ModLoader, /*IILookup=*/nullptr,
+                  /*OwnsHeaderSearch=*/false);
+  PP.Initialize(*Target);
+
+  class EmbedFileNotFoundCallbacks : public PPCallbacks {
+  public:
+    bool EmbedFileNotFound(StringRef FileName) override { return true; }
+  };
+
+  PP.addPPCallbacks(std::make_unique<EmbedFileNotFoundCallbacks>());
+  PP.addPPCallbacks(std::make_unique<EmbedFileNotFoundCallbacks>());
+
+  // Lex source text.
+  PP.EnterMainSourceFile();
+  PP.LexTokensUntilEOF();
+
+  ASSERT_EQ(0u, DiagConsumer->getNumErrors());
+}
+
 TEST_F(PPCallbacksTest, OpenCLExtensionPragmaEnabled) {
   const char* Source =
     "#pragma OPENCL EXTENSION cl_khr_fp64 : enable\n";

[cofibrant]

I've added a unit test, but had to make a small tweak to the logic in HandleDirective() to avoid unwrapping an empty optional