[clang-scan-deps] Fix edge cases in the minimizer

[aganea]

Summary

This patch fixes:

1. Invisible characters that come just before #include, such as #ifndef. ( were hidden, depending on the display locale). I choose to simply skip over non-ASCII characters.
2. Double slashes in #include directive with angle brackets not handled correctly: #include <dir//file.h>
3. #error directive with quoted, multi-line content, along with CR+LF line endings wasn't handled correctly.

[arphaman]

Thanks for figuring out these issues! It looks like you're fixing several edge cases, would you mind splitting the patch up for easier review?

Regarding the invisible characters before #ifdef, are you sure that's the right behavior? If you run the preprocessor over your test case, it doesn't accept it as a valid input, so why should the minimizer be different:

$ clang -cc1 -DTEST  %t.c -Eonly -o -
%t.c:3:2: error: #endif without #if
#endif
 ^
1 error generated.

[aganea]

In D65906#1620034, @arphaman wrote:

Regarding the invisible characters before #ifdef, are you sure that's the right behavior?

Should we then copy these invisible characters to the minimized output? Believe it or not, these characters are there in our codebase since 2013, and never clang has complained about it :)

[dexonsmith]

In D65906#1621542, @aganea wrote:

In D65906#1620034, @arphaman wrote:

Regarding the invisible characters before #ifdef, are you sure that's the right behavior?

Should we then copy these invisible characters to the minimized output? Believe it or not, these characters are there in our codebase since 2013, and never clang has complained about it :)

Do you have an example command-line where clang doesn't error with your test file? Alex gave you an example where it does error.

[arphaman]

@aganea These are not just any invisible characters that you have, this is the UTF8 BOM. Clang's Lexer skips over them if they're in the beginning of the file (Lexer::InitLexer). The minimizer should do the same thing, so ideally you would factor out the BOM detection and teach the minimizer to skip past it.

[aganea]

In D65906#1622209, @arphaman wrote:

@aganea These are not just any invisible characters that you have, this is the UTF8 BOM. Clang's Lexer skips over them if they're in the beginning of the file (Lexer::InitLexer). The minimizer should do the same thing, so ideally you would factor out the BOM detection and teach the minimizer to skip past it.

Thanks for the heads up. I'll do that.

[aganea]

Please see the other reviews I've sent.

lib/Lex/DependencyDirectivesSourceMinimizer.cpp

bool Minimizer::reportError(const char *CurPtr, unsigned Err) {
   if (!Diags)
     return true;
   assert(CurPtr >= Input.data() && "invalid buffer ptr");
   Diags->Report(InputSourceLoc.getLocWithOffset(CurPtr - Input.data()), Err);
   return true;
 }
 
 static void skipOverSpaces(const char *&First, const char *const End) {
-  while (First != End && isHorizontalWhitespace(*First))
+  while (First != End &&
+         (isHorizontalWhitespace(*First) || !clang::isASCII(*First)))
     ++First;
 }
 
 LLVM_NODISCARD static bool isRawStringLiteral(const char *First,
                                               const char *Current) {
   assert(First <= Current);
 
   // Check if we can even back up.
for (;;) {
     if (*Last != '"')
       continue;
     First = Last + 1;
     return;
   }
 }
 
 static void skipString(const char *&First, const char *const End) {
-  assert(*First == '\'' || *First == '"');
-  const char Terminator = *First;
+  assert(*First == '\'' || *First == '"' || *First == '<');
+  const char Terminator = *First == '<' ? '>' : *First;
   for (++First; First != End && *First != Terminator; ++First)
     if (*First == '\\')
       if (++First == End)
         return;
   if (First != End)
     ++First; // Finish off the string.
 }
 
-static void skipNewline(const char *&First, const char *End) {
-  assert(isVerticalWhitespace(*First));
-  ++First;
+// Returns the length of EOL, either 0 (no end-of-line), 1 (\n) or 2 (\r\n)
+static unsigned isEOL(const char *First, const char *const End) {
   if (First == End)
-    return;
+    return 0;
+  if (End - First > 1) {
+    if (isVerticalWhitespace(First[0]) && isVerticalWhitespace(First[1]) &&
+        First[0] != First[1])
+      return 2;
+  }
+  return !!isVerticalWhitespace(First[0]);
+}
 
-  // Check for "\n\r" and "\r\n".
-  if (LLVM_UNLIKELY(isVerticalWhitespace(*First) && First[-1] != First[0]))
-    ++First;
+static unsigned skipNewline(const char *&First, const char *End) {
+  unsigned Len = isEOL(First, End);
+  assert(Len);
+  First += Len;
+  return Len;
+}
+
+static bool wasLineContinuation(const char *First, unsigned Len) {
+  return First[-(int)Len - 1] == '\\';
 }
 
 static void skipToNewlineRaw(const char *&First, const char *const End) {
   for (;;) {
     if (First == End)
       return;
 
-    if (isVerticalWhitespace(*First))
+    unsigned Len = isEOL(First, End);
+    if (Len)
       return;
 
-    while (!isVerticalWhitespace(*First))
+    do {
       if (++First == End)
         return;
+      Len = isEOL(First, End);
+    } while (!Len);
 
-    if (First[-1] != '\\')
+    First += Len;
+
+    if (!wasLineContinuation(First, Len))
       return;
 
-    ++First; // Keep going...
+    // Keep skipping lines...
   }
 }
 
 static const char *reverseOverSpaces(const char *First, const char *Last) {
   assert(First <= Last);
   while (First != Last && isHorizontalWhitespace(Last[-1]))
     --Last;
   return Last;
if (Prev == '8' && (Cur - 1 != Start) && *(Cur - 2) == 'u')
     return false;
   if (!isPreprocessingNumberBody(Prev))
     return false;
   // The next character should be a valid identifier body character.
   return (Cur + 1) < End && isIdentifierBody(*(Cur + 1));
 }
 
 static void skipLine(const char *&First, const char *const End) {
-  do {
+  for (;;) {
     assert(First <= End);
     if (First == End)
       return;
 
     if (isVerticalWhitespace(*First)) {
       skipNewline(First, End);
       return;
     }
while (First != End && !isVerticalWhitespace(*First)) {
 
       // "/*...*/".
       skipBlockComment(First, End);
     }
     if (First == End)
       return;
 
     // Skip over the newline.
-    assert(isVerticalWhitespace(*First));
-    skipNewline(First, End);
-  } while (First[-2] == '\\'); // Continue past line-continuations.
+    unsigned Len = skipNewline(First, End);
+    if (!wasLineContinuation(First, Len)) // Continue past line-continuations.
+      break;
+  }
 }
 
 static void skipDirective(StringRef Name, const char *&First,
                           const char *const End) {
   if (llvm::StringSwitch<bool>(Name)
           .Case("warning", true)
           .Case("error", true)
           .Default(false))
do {
       // Iterate over strings correctly to avoid comments and newlines.
       if (*Last == '"' || *Last == '\'') {
         if (LLVM_UNLIKELY(isRawStringLiteral(First, Last)))
           skipRawString(Last, End);
         else
           skipString(Last, End);
         continue;
       }
+      if (top() == pp_include && *Last == '<') {
+        skipString(Last, End);
+        continue;
+      }
       if (*Last != '/' || End - Last < 2) {
         ++Last;
         continue; // Gather the rest up to print verbatim.
       }
 
       if (Last[1] != '/' && Last[1] != '*') {
         ++Last;
         continue;
do {
       skipBlockComment(First, End);
       skipOverSpaces(First, End);
       Last = First;
     } while (Last != End && !isVerticalWhitespace(*Last));
 
     // Print out the string.
     if (Last == End || Last == First || Last[-1] != '\\') {
       append(First, reverseOverSpaces(First, Last));
+
+      First = Last;
+      skipNewline(First, End);
       return;
     }
 
     // Print up to the backslash, backing up over spaces.
     append(First, reverseOverSpaces(First, Last - 1));
 
     First = Last;
     skipNewline(First, End);

test/Lexer/minimize_source_to_dependency_directives_include.c

+// Test double slashes in #include directive along with angle brackets. Previously, this was interpreted as comments.
+// RUN: %clang_cc1 -DTEST -print-dependency-directives-minimized-source %s 2>&1 | FileCheck %s
+
+#include "a//b.h"
+#include <a//b.h>
+
+// CHECK: #include "a//b.h"
+// CHECK: #include <a//b.h>

test/Lexer/minimize_source_to_dependency_directives_invalid_chars.c

+// Test invisible, bad characters just before #ifdef
+// RUN: echo -n -e '\xef\xbb\xbf#ifdef TEST\n' > %t.c
+// RUN: echo '#include <string>' >> %t.c
+// RUN: echo '#endif' >> %t.c
+// RUN: %clang_cc1 -DTEST -print-dependency-directives-minimized-source %t.c 2>&1 | FileCheck %s
+
+// CHECK:      #ifdef TEST
+// CHECK-NEXT: #include <string>
+// CHECK-NEXT: #endif

test/Lexer/minimize_source_to_dependency_directives_invalid_error.c

+// Test CF+LF are properly handled along with quoted, multi-line #error
+// RUN: cat %s | unix2dos | %clang_cc1 -DOTHER -print-dependency-directives-minimized-source 2>&1 | FileCheck %s
+
+#ifndef TEST
+#error "message \
+   more message \
+   even more"
+#endif
+
+#ifdef OTHER
+#include <string>
+#endif
+
+// CHECK:      #ifdef OTHER
+// CHECK-NEXT: #include <string>
+// CHECK-NEXT: #endif