[clang-scan-deps] Fix "unterminated conditional directive" bug #146645

ziqingluo-90 · 2025-07-02T08:03:28Z

clang-scan-deps threw "unterminated conditional directive" error falsely on the following example:

#ifndef __TEST
#define __TEST

#if defined(__TEST_DUMMY)
 #if defined(__TEST_DUMMY2)
  #pragma GCC warning                                                          \
      "Hello!"
 #else
  #pragma GCC error                                                            \
      "World!"
 #endif // defined(__TEST_DUMMY2)
#endif  // defined(__TEST_DUMMY)

#endif // #ifndef __TEST

The issue comes from PR #143950, where the flag LastNonWhitespace does not correctly represent the state for the example above. The PR aimed to support that a line-continuation can be followed by whitespaces. This fix uses a more straightforward but less efficient way to do the same thing---it looks back for a line-continuation and skips any number of whitespaces before that.

rdar://153742186

llvmbot · 2025-07-02T08:04:00Z

@llvm/pr-subscribers-clang

Author: Ziqing Luo (ziqingluo-90)

Changes

clang-scan-deps threw "unterminated conditional directive" error falsely on the following example:

#ifndef __TEST
#define __TEST

#if defined(__TEST_DUMMY)
 #if defined(__TEST_DUMMY2)
  #pragma GCC warning                                                          \
      "Hello!"
 #else
  #pragma GCC error                                                            \
      "World!"
 #endif // defined(__TEST_DUMMY2)
#endif  // defined(__TEST_DUMMY)

#endif // #ifndef __TEST

The issue comes from PR #143950, where the flag LastNonWhitespace does not correctly represent the state for the example above. The PR aimed to support that a line-continuation can be followed by whitespaces. This fix uses a more straightforward but less efficient way to do the same thing---it looks back for a line-continuation and skips any number of whitespaces before that.

rdar://153742186

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

2 Files Affected:

(modified) clang/lib/Lex/DependencyDirectivesScanner.cpp (+21-8)
(modified) clang/unittests/Lex/DependencyDirectivesScannerTest.cpp (+31)

diff --git a/clang/lib/Lex/DependencyDirectivesScanner.cpp b/clang/lib/Lex/DependencyDirectivesScanner.cpp
index 1b6b16c561141..b1f1285bd959d 100644
--- a/clang/lib/Lex/DependencyDirectivesScanner.cpp
+++ b/clang/lib/Lex/DependencyDirectivesScanner.cpp
@@ -393,7 +393,7 @@ static bool isQuoteCppDigitSeparator(const char *const Start,
 }
 
 void Scanner::skipLine(const char *&First, const char *const End) {
-  char LastNonWhitespace = ' ';
+  const char * const OldFirst = First;
   for (;;) {
     assert(First <= End);
     if (First == End)
@@ -419,8 +419,6 @@ void Scanner::skipLine(const char *&First, const char *const End) {
       // Iterate over comments correctly.
       if (*First != '/' || End - First < 2) {
         LastTokenPtr = First;
-        if (!isWhitespace(*First))
-          LastNonWhitespace = *First;
         ++First;
         continue;
       }
@@ -433,8 +431,6 @@ void Scanner::skipLine(const char *&First, const char *const End) {
 
       if (First[1] != '*') {
         LastTokenPtr = First;
-        if (!isWhitespace(*First))
-          LastNonWhitespace = *First;
         ++First;
         continue;
       }
@@ -446,9 +442,26 @@ void Scanner::skipLine(const char *&First, const char *const End) {
       return;
 
     // Skip over the newline.
-    skipNewline(First, End);
-
-    if (LastNonWhitespace != '\\')
+    auto Len = skipNewline(First, End);
+    // Since P2223R2 allows the line-continuation slash \ to be followed by
+    // additional whitespace, we need to check that here if `First`
+    // follows a `\\` and whitespaces.
+
+    // `LookBack` points to the character before the newline:
+    const char *LookBack = First - Len;
+    bool LineContinuationFound = false;
+
+    // Move `LookBack` backwards to find line-continuation and whitespaces:
+    while (LookBack >= OldFirst) { // bound `LookBack` by `OldFirst`:
+      if (isWhitespace(*LookBack)) {
+        --LookBack; // whitespace before '\\' is ok
+        continue;
+      }
+      if (*LookBack == '\\')
+        LineContinuationFound = true;
+      break; // not a whitespace, end loop
+    }
+    if (!LineContinuationFound)
       break;
   }
 }
diff --git a/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp b/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
index 61f74929c1e98..4f64ed80cb7bf 100644
--- a/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
+++ b/clang/unittests/Lex/DependencyDirectivesScannerTest.cpp
@@ -880,6 +880,37 @@ TEST(MinimizeSourceToDependencyDirectivesTest,
   EXPECT_EQ(pp_eof, Directives[22].Kind);
 }
 
+TEST(MinimizeSourceToDependencyDirectivesTest,
+     TestFixedBugThatReportUnterminatedDirectiveFalsely) {
+  SmallVector<char, 512> Out;
+  SmallVector<dependency_directives_scan::Token, 16> Tokens;
+  SmallVector<Directive, 16> Directives;
+
+  StringRef Input = "#ifndef __TEST \n"
+                    "#define __TEST \n"
+                    "#if defined(__TEST_DUMMY) \n"
+                    "#if defined(__TEST_DUMMY2) \n"
+                    "#pragma GCC warning        \\ \n"
+                    "\"hello!\"\n"
+                    "#else\n"
+                    "#pragma GCC error          \\ \n"
+                    "\"world!\" \n"
+                    "#endif // defined(__TEST_DUMMY2) \n"
+                    "#endif  // defined(__TEST_DUMMY) \n"
+                    "#endif // #ifndef __TEST \n";
+  ASSERT_FALSE( // False on no error:
+      minimizeSourceToDependencyDirectives(Input, Out, Tokens, Directives));
+  ASSERT_TRUE(Directives.size() == 8);
+  EXPECT_EQ(pp_ifndef, Directives[0].Kind);
+  EXPECT_EQ(pp_define, Directives[1].Kind);
+  EXPECT_EQ(pp_if, Directives[2].Kind);
+  EXPECT_EQ(pp_if, Directives[3].Kind);
+  EXPECT_EQ(pp_endif, Directives[4].Kind);
+  EXPECT_EQ(pp_endif, Directives[5].Kind);
+  EXPECT_EQ(pp_endif, Directives[6].Kind);
+  EXPECT_EQ(pp_eof, Directives[7].Kind);
+}
+
 TEST(MinimizeSourceToDependencyDirectivesTest, PoundWarningAndError) {
   SmallVector<char, 128> Out;

github-actions · 2025-07-02T08:07:00Z

✅ With the latest revision this PR passed the C/C++ code formatter.

ziqingluo-90 · 2025-07-02T08:07:47Z

clang/unittests/Lex/DependencyDirectivesScannerTest.cpp

+                    "#if defined(__TEST_DUMMY2) \n"
+                    "#pragma GCC warning        \\ \n"
+                    "\"hello!\"\n"
+                    "#else\n"


The #else directive was and is still missed by the dep-scan but I think it is irrelevant to this fix.

I'm pretty sure that everything here should be removed except for:

#ifndef __TEST #define __TEST #endif

But I don't think that matters for this fix.

naveen-seth

I realized that I probably should have also removed the following code in #143950:

llvm-project/clang/lib/Lex/DependencyDirectivesScanner.cpp

Lines 445 to 451 in 2b03efc

    
           // Continue on the same line if an EOL is preceded with backslash 
        
           if (First + 1 < End && *First == '\\') { 
        
             if (unsigned Len = isEOL(First + 1, End)) { 
        
               First += 1 + Len; 
        
               continue; 
        
             } 
        
           }

The logic added in #143950 also handles line splices with 0 trailing whitespace characters, so this code is now redundant. (When removing it, the test run fine too.)

Should removing this code be a separate PR?

clang/lib/Lex/DependencyDirectivesScanner.cpp

`clang-scan-deps` threw "unterminated conditional directive" error falsely on the following example: ``` #if defined(__TEST_DUMMY2) #pragma GCC warning \ "Hello!" #else #pragma GCC error \ "World!" #endif // defined(__TEST_DUMMY2) ``` The issue comes from PR llvm#143950, where the flag `LastNonWhitespace` does not correctly represent the state in the example above. The PR aimed to support that a line-continuation can be followed by whitespaces. This fix uses a more straightforward but less efficient way to do the same thing---it looks back for a line-continuation and skips any number of whitespaces before that. rdar://153742186

ziqingluo-90 · 2025-07-03T08:04:49Z

The logic added in #143950 also handles line splices with 0 trailing whitespace characters, so this code is now redundant. (When removing it, the test run fine too.)

Should removing this code be a separate PR?

Done

ziqingluo-90 requested review from Bigcheese and naveen-seth July 2, 2025 08:03

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 2, 2025

ziqingluo-90 requested a review from hyp July 2, 2025 08:03

ziqingluo-90 commented Jul 2, 2025

View reviewed changes

naveen-seth reviewed Jul 2, 2025

View reviewed changes

clang/lib/Lex/DependencyDirectivesScanner.cpp Show resolved Hide resolved

ziqingluo-90 requested a review from ahatanak July 2, 2025 23:29

ziqingluo-90 added 3 commits July 3, 2025 15:25

fix format issue

be7e37d

Take naveen-seth's suggestion.

d48a44f

ziqingluo-90 force-pushed the ziqing/PR-153742186 branch from b6f626b to d48a44f Compare July 3, 2025 08:02

Bigcheese approved these changes Jul 3, 2025

View reviewed changes

ziqingluo-90 merged commit afd20aa into llvm:main Jul 4, 2025
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang-scan-deps] Fix "unterminated conditional directive" bug #146645

[clang-scan-deps] Fix "unterminated conditional directive" bug #146645

Uh oh!

ziqingluo-90 commented Jul 2, 2025

Uh oh!

llvmbot commented Jul 2, 2025

Uh oh!

github-actions bot commented Jul 2, 2025 •

edited

Loading

Uh oh!

ziqingluo-90 Jul 2, 2025 •

edited

Loading

Uh oh!

Bigcheese Jul 3, 2025 •

edited

Loading

Uh oh!

naveen-seth left a comment •

edited

Loading

Uh oh!

Uh oh!

ziqingluo-90 commented Jul 3, 2025

Uh oh!

Uh oh!

Uh oh!

	// Continue on the same line if an EOL is preceded with backslash
	if (First + 1 < End && *First == '\\') {
	if (unsigned Len = isEOL(First + 1, End)) {
	First += 1 + Len;
	continue;
	}
	}

[clang-scan-deps] Fix "unterminated conditional directive" bug #146645

[clang-scan-deps] Fix "unterminated conditional directive" bug #146645

Uh oh!

Conversation

ziqingluo-90 commented Jul 2, 2025

Uh oh!

llvmbot commented Jul 2, 2025

Uh oh!

github-actions bot commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ziqingluo-90 Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

The #else directive was and is still missed by the dep-scan but I think it is irrelevant to this fix.

Uh oh!

Bigcheese Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naveen-seth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ziqingluo-90 commented Jul 3, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 2, 2025 •

edited

Loading

ziqingluo-90 Jul 2, 2025 •

edited

Loading

The `#else` directive was and is still missed by the `dep-scan` but I think it is irrelevant to this fix.

Bigcheese Jul 3, 2025 •

edited

Loading

naveen-seth left a comment •

edited

Loading