[clang] Correct -frewrite-includes generation of line control directives with mixed EOL forms.

[tahonermann]

Summary

Previously, if a header file and a source file used different end of line (EOL) forms, preprocessed output generated with the -frewrite-includes option would, in some cases, generate line control directives with the wrong line number due to an error in how source file lines were counted.

Fixes https://github.com/llvm/llvm-project/issues/59736

[tahonermann]

clang/lib/Frontend/Rewrite/InclusionRewriter.cpp

292–303

The issue here was that, when the last line of TextToWrite was not empty and did not end with LocalEOL, Line would be incremented at line 298 despite no EOL sequence being written at either line 300 or line 303.

The rewrite avoids use of StringRef::split() so as to avoid the ambiguity created when Rest ends with LocalEOL (Rest ends up empty regardless). I think this simplifies the code by removing the need for differentiated handling of EnsureNewLine in each of the branches of the if statement.

[cor3ntin]

Thanks Tom

I think this looks reasonable but given the subtlety a comment in the code may be useful
+ a release note to indicate the fixed issue

[tahonermann]

Addressed Corentin's review feedback. Thank you, Corentin! I'm going to remember to update the release notes doc all on my own one of these days, I'm sure of it! :)

clang/docs/ReleaseNotes.rst

 - Fix bug where constant evaluation treated a pointer to member that points to
   a weak member as never being null. Such comparisons are now treated as
   non-constant.
 - Fix sanity check when value initializing an empty union so that it takes into
   account anonymous structs which is a GNU extension. This fixes
   `Issue 58800 <https://github.com/llvm/llvm-project/issues/58800>`_
 - Fix an issue that triggers a crash if we instantiate a hidden friend functions.
   This fixes `Issue 54457 <https://github.com/llvm/llvm-project/issues/54457>`_
+- Fix an issue where -frewrite-includes generated line control directives with
+  incorrect line numbers in some cases when a header file used an end of line
+  character sequence that differed from the primary source file.
+  `Issue 59736 <https://github.com/llvm/llvm-project/issues/59736>`_
 
 Improvements to Clang's diagnostics
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Clang will now check compile-time determinable string literals as format strings.
   Fixes `Issue 55805: <https://github.com/llvm/llvm-project/issues/55805>`_.
 - ``-Wformat`` now recognizes ``%b`` for the ``printf``/``scanf`` family of
   functions and ``%B`` for the ``printf`` family of functions. Fixes
   `Issue 56885: <https://github.com/llvm/llvm-project/issues/56885>`_.

clang/lib/Frontend/Rewrite/InclusionRewriter.cpp

void InclusionRewriter::OutputContentUpTo(const MemoryBufferRef &FromFile,
   // one byte is safe.
   if (LocalEOL.size() == 2 &&
       LocalEOL[0] == (FromFile.getBufferStart() + WriteTo)[-1] &&
       LocalEOL[1] == (FromFile.getBufferStart() + WriteTo)[0])
     WriteTo++;
 
   StringRef TextToWrite(FromFile.getBufferStart() + WriteFrom,
                         WriteTo - WriteFrom);
+  // count lines manually, it's faster than getPresumedLoc()
+  Line += TextToWrite.count(LocalEOL);
 
   if (MainEOL == LocalEOL) {
     OS << TextToWrite;
-    // count lines manually, it's faster than getPresumedLoc()
-    Line += TextToWrite.count(LocalEOL);
-    if (EnsureNewline && !TextToWrite.endswith(LocalEOL))
-      OS << MainEOL;
   } else {
     // Output the file one line at a time, rewriting the line endings as we go.
     StringRef Rest = TextToWrite;
     while (!Rest.empty()) {
-      StringRef LineText;
-      std::tie(LineText, Rest) = Rest.split(LocalEOL);
+      // Identify and output the next line excluding an EOL sequence if present.
+      size_t Idx = Rest.find(LocalEOL);
+      StringRef LineText = Rest.substr(0, Idx);
       OS << LineText;
-      Line++;
-      if (!Rest.empty())
+      if (Idx != StringRef::npos) {
+        // An EOL sequence was present, output the EOL sequence for the
+        // main source file and skip past the local EOL sequence.
         OS << MainEOL;
+        Idx += LocalEOL.size();
+      }
+      // Strip the line just handled. If Idx is npos or matches the end of the
+      // text, Rest will be set to an empty string and the loop will terminate.
+      Rest = Rest.substr(Idx);
     }
-    if (TextToWrite.endswith(LocalEOL) || EnsureNewline)
-      OS << MainEOL;
   }
+  if (EnsureNewline && !TextToWrite.endswith(LocalEOL))
+    OS << MainEOL;
+
   WriteFrom = WriteTo;
 }
 
 /// Print characters from \p FromFile starting at \p NextToWrite up until the
 /// inclusion directive at \p StartToken, then print out the inclusion
 /// inclusion directive disabled by a #if directive, updating \p NextToWrite
 /// and \p Line to track the number of source lines visited and the progress
 /// through the \p FromFile buffer.

clang/test/Frontend/rewrite-includes-mixed-eol-crlf.h

+// Note: This header file has CRLF line endings.
+// The indentation in some of the conditional inclusion directives below is
+// intentional and is required for this test to function as a regression test
+// for GH59736.
+_Static_assert(__LINE__ == 5, "");
+#if 1
+_Static_assert(__LINE__ == 7, "");
+  #if 1
+  _Static_assert(__LINE__ == 9, "");
+  #endif
+#endif

clang/test/Frontend/rewrite-includes-mixed-eol-crlf.c

+// RUN: %clang_cc1 -E -frewrite-includes %s | %clang_cc1 -
+// expected-no-diagnostics
+// Note: This source file has CRLF line endings.
+// This test validates that -frewrite-includes translates the end of line (EOL)
+// form used in header files to the EOL form used in the the primary source
+// file when the files use different EOL forms.
+#include "rewrite-includes-mixed-eol-crlf.h"
+#include "rewrite-includes-mixed-eol-lf.h"

clang/test/Frontend/rewrite-includes-mixed-eol-lf.h

+// Note: This header file has LF line endings.
+// The indentation in some of the conditional inclusion directives below is
+// intentional and is required for this test to function as a regression test
+// for GH59736.
+_Static_assert(__LINE__ == 5, "");
+#if 1
+_Static_assert(__LINE__ == 7, "");
+  #if 1
+  _Static_assert(__LINE__ == 9, "");
+  #endif
+#endif

clang/test/Frontend/rewrite-includes-mixed-eol-lf.c

+// RUN: %clang_cc1 -E -frewrite-includes %s | %clang_cc1 -
+// expected-no-diagnostics
+// Note: This source file has LF line endings.
+// This test validates that -frewrite-includes translates the end of line (EOL)
+// form used in header files to the EOL form used in the the primary source
+// file when the files use different EOL forms.
+#include "rewrite-includes-mixed-eol-crlf.h"
+#include "rewrite-includes-mixed-eol-lf.h"