[clang] Fix static analyzer concerns in #embed code

[Fznamznon]

1. Dead code in LookupEmbedFile. The loop always exited on the first iteration. This was also causing a bug of not checking all directories provided by --embed-dir.

2. Use of uninitialized variable CurTok in LexEmbedParameters. It was used to initialize the field which seems to be unused. Removed unused field, this way CurTok should be initialized by Lex method.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Mariya Podchishchaeva (Fznamznon)

Changes

1. Dead code int LookupEmbedFile. The loop always exited on the first iteration. This was also causing a bug of not checking all directories provided by --embed-dir

2. Use of uninitialized variable CurTok LexEmbedParameters. It was used to initialize the field which seems to be unused. Removed unused field, this way CurTok should be initialized by Lex method.

---

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

3 Files Affected:

• (modified) clang/include/clang/Lex/PPEmbedParameters.h (-1)
• (modified) clang/lib/Lex/PPDirectives.cpp (+3-5)
• (modified) clang/test/Preprocessor/embed_file_not_found_chevron.c (+2-1)

diff --git a/clang/include/clang/Lex/PPEmbedParameters.h b/clang/include/clang/Lex/PPEmbedParameters.h
index 51bf908524e7a..c4fb8d02f6f35 100644
--- a/clang/include/clang/Lex/PPEmbedParameters.h
+++ b/clang/include/clang/Lex/PPEmbedParameters.h
@@ -75,7 +75,6 @@ struct LexEmbedParametersResult {
   std::optional<PPEmbedParameterIfEmpty> MaybeIfEmptyParam;
   std::optional<PPEmbedParameterPrefix> MaybePrefixParam;
   std::optional<PPEmbedParameterSuffix> MaybeSuffixParam;
-  SourceRange ParamRange;
   int UnrecognizedParams;
 
   size_t PrefixTokenCount() const {
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index a53540b12dee6..4e77df9ec444c 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -1137,7 +1137,9 @@ Preprocessor::LookupEmbedFile(StringRef Filename, bool isAngled, bool OpenFile,
     SeparateComponents(LookupPath, Entry, Filename, false);
     llvm::Expected<FileEntryRef> ShouldBeEntry =
         FM.getFileRef(LookupPath, OpenFile);
-    return llvm::expectedToOptional(std::move(ShouldBeEntry));
+    if (ShouldBeEntry)
+      return llvm::expectedToOptional(std::move(ShouldBeEntry));
+    llvm::consumeError(ShouldBeEntry.takeError());
   }
   return std::nullopt;
 }
@@ -3624,12 +3626,10 @@ Preprocessor::LexEmbedParameters(Token &CurTok, bool ForHasEmbed) {
   LexEmbedParametersResult Result{};
   SmallVector<Token, 2> ParameterTokens;
   tok::TokenKind EndTokenKind = ForHasEmbed ? tok::r_paren : tok::eod;
-  Result.ParamRange = {CurTok.getLocation(), CurTok.getLocation()};
 
   auto DiagMismatchedBracesAndSkipToEOD =
       [&](tok::TokenKind Expected,
           std::pair<tok::TokenKind, SourceLocation> Matches) {
-        Result.ParamRange.setEnd(CurTok.getEndLoc());
         Diag(CurTok, diag::err_expected) << Expected;
         Diag(Matches.second, diag::note_matching) << Matches.first;
         if (CurTok.isNot(tok::eod))
@@ -3638,7 +3638,6 @@ Preprocessor::LexEmbedParameters(Token &CurTok, bool ForHasEmbed) {
 
   auto ExpectOrDiagAndSkipToEOD = [&](tok::TokenKind Kind) {
     if (CurTok.isNot(Kind)) {
-      Result.ParamRange.setEnd(CurTok.getEndLoc());
       Diag(CurTok, diag::err_expected) << Kind;
       if (CurTok.isNot(tok::eod))
         DiscardUntilEndOfDirective(CurTok);
@@ -3872,7 +3871,6 @@ Preprocessor::LexEmbedParameters(Token &CurTok, bool ForHasEmbed) {
       }
     }
   }
-  Result.ParamRange.setEnd(CurTok.getLocation());
   return Result;
 }
 
diff --git a/clang/test/Preprocessor/embed_file_not_found_chevron.c b/clang/test/Preprocessor/embed_file_not_found_chevron.c
index 472222aafa55a..b6d110f452c09 100644
--- a/clang/test/Preprocessor/embed_file_not_found_chevron.c
+++ b/clang/test/Preprocessor/embed_file_not_found_chevron.c
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -std=c23 %s -E -verify
+// RUN: %clang_cc1 -std=c23 %s -E -verify --embed-dir=%S --embed-dir=%S/Inputs
 
+#embed <jk.txt>
 #embed <nfejfNejAKFe>
 // expected-error@-1 {{'nfejfNejAKFe' file not found}}

[AaronBallman]

LGTM but a question about whether a test should be split off or not.

[AaronBallman]

It feels like this test is unrelated to the test name now; should we move this into its own test file named something like embed_search_paths.c or something?

[Fznamznon]

Makes sense.

[AaronBallman]

LGTM!