[clang][deps] Stop lexing if hit a failure while loading a PCH/module in a submodule.

[vsapsai]

Otherwise we are continuing in an invalid state and can easily crash.

It is a follow-up to cde90e6 but an important difference is when a failure happens in a submodule. In this case in Preprocessor::HandleEndOfFile tok::eof is replaced by tok::annot_module_end. And after exiting a file with bad #include/#import we work with a new buffer, so BufferPtr < BufferEnd. As there are no signs to stop lexing we just keep doing it.

The fix is the same as in dc9fdaf in Lexer::LexTokenInternal but this time in Lexer::LexDependencyDirectiveToken as well.

rdar://152499276

[llvmbot]

@llvm/pr-subscribers-clang

Author: Volodymyr Sapsai (vsapsai)

Changes

Otherwise we are continuing in an invalid state and can easily crash.

It is a follow-up to cde90e6 but an important difference is when a failure happens in a submodule. In this case in Preprocessor::HandleEndOfFile tok::eof is replaced by tok::annot_module_end. And after exiting a file with bad #include/#import we work with a new buffer, so BufferPtr < BufferEnd. As there are no signs to stop lexing we just keep doing it.

The fix is the same as in dc9fdaf in Lexer::LexTokenInternal but this time in Lexer::LexDependencyDirectiveToken as well.

rdar://152499276

---

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

2 Files Affected:

• (modified) clang/lib/Lex/Lexer.cpp (+3)
• (added) clang/test/ClangScanDeps/fatal-module-loader-error.m (+42)

diff --git a/clang/lib/Lex/Lexer.cpp b/clang/lib/Lex/Lexer.cpp
index 30ac48b8010b8..8a0560615bf97 100644
--- a/clang/lib/Lex/Lexer.cpp
+++ b/clang/lib/Lex/Lexer.cpp
@@ -4589,6 +4589,9 @@ bool Lexer::LexDependencyDirectiveToken(Token &Result) {
 
   if (Result.is(tok::hash) && Result.isAtStartOfLine()) {
     PP->HandleDirective(Result);
+    if (PP->hadModuleLoaderFatalFailure())
+      // With a fatal failure in the module loader, we abort parsing.
+      return true;
     return false;
   }
   if (Result.is(tok::raw_identifier)) {
diff --git a/clang/test/ClangScanDeps/fatal-module-loader-error.m b/clang/test/ClangScanDeps/fatal-module-loader-error.m
new file mode 100644
index 0000000000000..ed4d8628a6471
--- /dev/null
+++ b/clang/test/ClangScanDeps/fatal-module-loader-error.m
@@ -0,0 +1,42 @@
+// This tests that after a fatal module loader error, we do not continue parsing.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: mkdir %t/ModulesCache
+// RUN: touch %t/ModulesCache/Unusable.pcm
+
+// RUN: not clang-scan-deps -format experimental-full -mode preprocess-dependency-directives -- \
+// RUN:   %clang -fmodules -fimplicit-modules -Xclang -fdisable-module-hash -fmodules-cache-path=%t/ModulesCache \
+// RUN:   -F %t/Frameworks -c %t/test.m -o %t/test.o
+// RUN: ls %t/ModulesCache | not grep AfterUnusable
+
+//--- Frameworks/Unusable.framework/Headers/Unusable.h
+// empty
+//--- Frameworks/Unusable.framework/Modules/module.modulemap
+framework module Unusable { header "Unusable.h" }
+
+//--- Frameworks/AfterUnusable.framework/Headers/AfterUnusable.h
+// empty
+//--- Frameworks/AfterUnusable.framework/Modules/module.modulemap
+framework module AfterUnusable { header "AfterUnusable.h" }
+
+//--- Frameworks/Importer.framework/Headers/Importer.h
+#import <Importer/ImportUnusable.h>
+// Parsing should have stopped and we should not handle AfterUnusable.
+#import <AfterUnusable/AfterUnusable.h>
+
+//--- Frameworks/Importer.framework/Headers/ImportUnusable.h
+// It is important that this header is a submodule.
+#import <Unusable/Unusable.h>
+// Parsing should have stopped and we should not handle AfterUnusable.
+#import <AfterUnusable/AfterUnusable.h>
+
+//--- Frameworks/Importer.framework/Modules/module.modulemap
+framework module Importer {
+  umbrella header "Importer.h"
+  module * { export * }
+  export *
+}
+
+//--- test.m
+#import <Importer/Importer.h>

[vsapsai]

Here true means "stop lexing" and it is interpreted in the loop

  while (!CurLexerCallback(*this, Result))
    ;

in Preprocessor::Lex.

[jansvoboda11]

LGTM, thanks!

[shafik]

Seems like this should have a release note?

[vsapsai]

Seems like this should have a release note?

What is the bar for a change to be release noted? This change is pretty much "make clang-scan-deps less likely to crash" and personally I don't think it deserves a release note.

[shafik]

Seems like this should have a release note?

What is the bar for a change to be release noted? This change is pretty much "make clang-scan-deps less likely to crash" and personally I don't think it deserves a release note.

If a user can see different behavior after the change then we should document that. I did not notice this was a follow-up change but the previous change also did not have a release note.

If this is completely an internal change and only llvm folks will see a different, then sure but at least it looks like from the test that this will cause different behavior for users.

CC @AaronBallman

[AaronBallman]

Seems like this should have a release note?

What is the bar for a change to be release noted? This change is pretty much "make clang-scan-deps less likely to crash" and personally I don't think it deserves a release note.

If a user can see different behavior after the change then we should document that. I did not notice this was a follow-up change but the previous change also did not have a release note.

If this is completely an internal change and only llvm folks will see a different, then sure but at least it looks like from the test that this will cause different behavior for users.

CC @AaronBallman

We've got some docs on this, but that's basically the bar. Almost every PR should have a release note unless the PR is NFC or modifying something that hasn't yet been released (and doesn't require the existing release note to be updated). Fixing a crashing bug is something we should tell users about; it's good to recognize when we're improving quality as opposed to just adding new functionality.