[clang-scan-deps] Minimizer: Correctly handle multi-line content with CR+LF line endings

[aganea]

Summary

Previously, an #error directive with quoted, multi-line content, along with CR+LF line endings wasn't handled correctly.

[aganea]

Use proper test file.

[arphaman]

lib/Lex/DependencyDirectivesSourceMinimizer.cpp

213

Nit: Please add && "expected newline") to the assert.

240

This return now changes behavior of skipToNewlineRaw for the clients, as First is now right after the newline if this return is taken, as opposed to right before the newline, like it used to be before this patch. I think you need to move First += Len after this return.

[aganea]

As requested.

[aganea]

Fixed unit tests.

[arphaman]

LGTM

[aganea]

This failed the build - I've changed unix2dos to svn:eol-style CRLF instead.

[arphaman]

Will the git monorepo handle svn:eol-style correctly?

[dexonsmith]

In D66556#1646229, @aganea wrote:

This failed the build - I've changed unix2dos to svn:eol-style CRLF instead.

Can we count on tr when we have shell? If so, I suggest this instead:

// REQUIRES: shell
// RUN: tr '\n' '\r\n' <%s | %clang_cc1 -DOTHER -print-dependency-directives-minimized-source 2>&1 | FileCheck %s

[dexonsmith]

In D66556#1647762, @dexonsmith wrote:

In D66556#1646229, @aganea wrote:

This failed the build - I've changed unix2dos to svn:eol-style CRLF instead.

Can we count on tr when we have shell? If so, I suggest this instead:

// REQUIRES: shell
// RUN: tr '\n' '\r\n' <%s | %clang_cc1 -DOTHER -print-dependency-directives-minimized-source 2>&1 | FileCheck %s

Eh, nevermind, tr doesn't handle multi-character replacements. Better to answer Alex's question.

[aganea]

In D66556#1647669, @arphaman wrote:

Will the git monorepo handle svn:eol-style correctly?

Other files in the repo already use svn:eol-style. I'm not sure what happens with SVN properties when commits are replicated in git. @jyknight @rnk ? I've added .gitattributes to ensure these files are checked out from git with the proper line ending.

[rnk]

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

[dexonsmith]

In D66556#1648109, @rnk wrote:

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

SGTM.

[hans]

In D66556#1648118, @dexonsmith wrote:

In D66556#1648109, @rnk wrote:

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

SGTM.

This broke users of the monorepo, where the file would show up as having changed locally, and with no way to reset it. I'm guessing that's because it was checked in with LF endings and then because of the .gitattributes file, it changes at checkout. I think the correct solution would be to check in the file with CRLF endings and not set any attributes or stuff. (Though this is all super confusing and I might have got it wrong.)

I've deleted the test file in r370175 to unblock development in the meantime.

[aganea]

In D66556#1648591, @hans wrote:

In D66556#1648118, @dexonsmith wrote:

In D66556#1648109, @rnk wrote:

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

SGTM.

This broke users of the monorepo, where the file would show up as having changed locally, and with no way to reset it. I'm guessing that's because it was checked in with LF endings and then because of the .gitattributes file, it changes at checkout. I think the correct solution would be to check in the file with CRLF endings and not set any attributes or stuff. (Though this is all super confusing and I might have got it wrong.)

I've deleted the test file in r370175 to unblock development in the meantime.

It already had CRLF endings locally before commit.
It is strange, the file shows up as having CRLF endings in the old revision in git, before the revert: https://raw.githubusercontent.com/llvm/llvm-project/9774a2ba279aea35f166b8ca489d0e8292026c38/clang/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
The same kind of problem occured in rL311683, later fixed by rL311732.
If you're fine with that, I'll do the same thing: remove .gitattributes and just re-commit the file minimize_source_to_dependency_directives_invalid_error.c with svn:eol-style CRLF.

[mstorsjo]

In D66556#1648591, @hans wrote:

In D66556#1648118, @dexonsmith wrote:

In D66556#1648109, @rnk wrote:

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

SGTM.

This broke users of the monorepo, where the file would show up as having changed locally, and with no way to reset it. I'm guessing that's because it was checked in with LF endings and then because of the .gitattributes file, it changes at checkout. I think the correct solution would be to check in the file with CRLF endings and not set any attributes or stuff. (Though this is all super confusing and I might have got it wrong.)

I've deleted the test file in r370175 to unblock development in the meantime.

Fwiw, this seems to me somewhat like a git bug; if I edit the .gitattributes file and comment out the problematic line, do a git diff, then uncomment the line again, then git diff suddenly shows no changes.

[hans]

In D66556#1648807, @aganea wrote:

In D66556#1648591, @hans wrote:

In D66556#1648118, @dexonsmith wrote:

In D66556#1648109, @rnk wrote:

I'm not sure what happens, but I see you added .gitattributes. I'd commit it as is. Buildbots using svn will keep working. You can check that the monorepo has the right line endings afterwards, and try again if not.

SGTM.

This broke users of the monorepo, where the file would show up as having changed locally, and with no way to reset it. I'm guessing that's because it was checked in with LF endings and then because of the .gitattributes file, it changes at checkout. I think the correct solution would be to check in the file with CRLF endings and not set any attributes or stuff. (Though this is all super confusing and I might have got it wrong.)

I've deleted the test file in r370175 to unblock development in the meantime.

It already had CRLF endings locally before commit.
It is strange, the file shows up as having CRLF endings in the old revision in git, before the revert: https://raw.githubusercontent.com/llvm/llvm-project/9774a2ba279aea35f166b8ca489d0e8292026c38/clang/test/Lexer/minimize_source_to_dependency_directives_invalid_error.c
The same kind of problem occured in rL311683, later fixed by rL311732.
If you're fine with that, I'll do the same thing: remove .gitattributes and just re-commit the file minimize_source_to_dependency_directives_invalid_error.c with svn:eol-style CRLF.

Yes, I think that should work. (But these things always break in surprising ways.)

[aganea]

rL370219 and rG3c307370c8f8

.gitattributes

+# Windows line ending tests
+test/Lexer/minimize_source_to_dependency_directives_invalid_error.c text eol=crlf
+test/FixIt/fixit-newline-style.c text eol=crlf
+test/Frontend/system-header-line-directive-ms-lineendings.c text eol=crlf

lib/Lex/DependencyDirectivesSourceMinimizer.cpp

static void skipString(const char *&First, const char *const End) {
   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 && 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;
+// Returns the length of the skipped newline
+static unsigned skipNewline(const char *&First, const char *End) {
+  if (First == End)
+    return 0;
+  assert(isVerticalWhitespace(*First));
+  unsigned Len = isEOL(First, End);
+  assert(Len && "expected newline");
+  First += Len;
+  return Len;
+}
+
+static bool wasLineContinuation(const char *First, unsigned EOLLen) {
+  return *(First - (int)EOLLen - 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] != '\\')
       return;
 
-    ++First; // Keep going...
+    First += Len;
+    // 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 {
       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_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
+// RUN: %clang_cc1 -DOTHER -print-dependency-directives-minimized-source %s 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