Don't crash when emitting fixits following Unicode characters.

This code is very sensitive to the difference between "columns" as printed
and "bytes" (SourceManager columns). All variables are now named explicitly
and our assumptions are (hopefully) documented as both comment and assertion.

Whether parseable fixits should use byte offsets or Unicode character counts
is pending discussion on the mailing list; currently the implementation uses
bytes (and has no problems on lines containing multibyte characters).
This has been added to the user manual.

<rdar://problem/11877454>

llvm-svn: 160319

clang/docs/UsersManual.html

@@ -229,6 +229,9 @@ print something like:
 
 <p>When this is disabled, Clang will print "test.c:28: warning..." with no
 column number.</p>
+
+<p>The printed column numbers count bytes from the beginning of the line; take
+care if your source contains multibyte characters.</p>
 </dd>
 
 <!-- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -->
@@ -395,6 +398,9 @@ exprs.c:47:15:{47:8-47:14}{47:17-47:24}: error: invalid operands to binary expre
 </pre>
 
 <p>The {}'s are generated by -fdiagnostics-print-source-range-info.</p>
+
+<p>The printed column numbers count bytes from the beginning of the line; take
+care if your source contains multibyte characters.</p>
 </dd>
 
 <!-- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -->
@@ -415,6 +421,9 @@ respectively). Both the file name and the insertion string escape backslash (as
 &quot;\\&quot;), tabs (as &quot;\t&quot;), newlines (as &quot;\n&quot;), double
 quotes(as &quot;\&quot;&quot;) and non-printable characters (as octal
 &quot;\xxx&quot;).</p>
+
+<p>The printed column numbers count bytes from the beginning of the line; take
+care if your source contains multibyte characters.</p>
 </dd>
 
 <dt id="opt_fno-elide-type">

clang/lib/Frontend/TextDiagnostic.cpp

@@ -1124,7 +1124,7 @@ std::string TextDiagnostic::buildFixItInsertionLine(
   std::string FixItInsertionLine;
   if (Hints.empty() || !DiagOpts.ShowFixits)
     return FixItInsertionLine;
-  unsigned PrevHintEnd = 0;
+  unsigned PrevHintEndCol = 0;
 
   for (ArrayRef<FixItHint>::iterator I = Hints.begin(), E = Hints.end();
        I != E; ++I) {
@@ -1136,11 +1136,15 @@ std::string TextDiagnostic::buildFixItInsertionLine(
       if (LineNo == SM.getLineNumber(HintLocInfo.first, HintLocInfo.second)) {
         // Insert the new code into the line just below the code
         // that the user wrote.
-        unsigned HintColNo
+        // Note: When modifying this function, be very careful about what is a
+        // "column" (printed width, platform-dependent) and what is a
+        // "byte offset" (SourceManager "column").
+        unsigned HintByteOffset
           = SM.getColumnNumber(HintLocInfo.first, HintLocInfo.second) - 1;
-        // hint must start inside the source or right at the end
-        assert(HintColNo<static_cast<unsigned>(map.bytes())+1);
-        HintColNo = map.byteToColumn(HintColNo);
+
+        // The hint must start inside the source or right at the end
+        assert(HintByteOffset < static_cast<unsigned>(map.bytes())+1);
+        unsigned HintCol = map.byteToColumn(HintByteOffset);
 
         // If we inserted a long previous hint, push this one forwards, and add
         // an extra space to show that this is not part of the previous
@@ -1149,32 +1153,27 @@ std::string TextDiagnostic::buildFixItInsertionLine(
         //
         // Note that if this hint is located immediately after the previous
         // hint, no space will be added, since the location is more important.
-        if (HintColNo < PrevHintEnd)
-          HintColNo = PrevHintEnd + 1;
+        if (HintCol < PrevHintEndCol)
+          HintCol = PrevHintEndCol + 1;
 
-        // FIXME: if the fixit includes tabs or other characters that do not
-        //  take up a single column per byte when displayed then
-        //  I->CodeToInsert.size() is not a column number and we're mixing
-        //  units (columns + bytes). We should get printable versions
-        //  of each fixit before using them.
-        unsigned LastColumnModified
-          = HintColNo + I->CodeToInsert.size();
-
-        if (LastColumnModified <= static_cast<unsigned>(map.bytes())) {
-          // If we're right in the middle of a multibyte character skip to
-          // the end of it.
-          while (map.byteToColumn(LastColumnModified) == -1)
-            ++LastColumnModified;
-          LastColumnModified = map.byteToColumn(LastColumnModified);
-        }
+        // FIXME: This function handles multibyte characters in the source, but
+        // not in the fixits. This assertion is intended to catch unintended
+        // use of multibyte characters in fixits. If we decide to do this, we'll
+        // have to track separate byte widths for the source and fixit lines.
+        assert((size_t)llvm::sys::locale::columnWidth(I->CodeToInsert) ==
+               I->CodeToInsert.size());
 
+        // This relies on one byte per column in our fixit hints.
+        // This should NOT use HintByteOffset, because the source might have
+        // Unicode characters in earlier columns.
+        unsigned LastColumnModified = HintCol + I->CodeToInsert.size();
         if (LastColumnModified > FixItInsertionLine.size())
           FixItInsertionLine.resize(LastColumnModified, ' ');
-        assert(HintColNo+I->CodeToInsert.size() <= FixItInsertionLine.size());
-        std::copy(I->CodeToInsert.begin(), I->CodeToInsert.end(),
-                  FixItInsertionLine.begin() + HintColNo);
 
-        PrevHintEnd = LastColumnModified;
+        std::copy(I->CodeToInsert.begin(), I->CodeToInsert.end(),
+                  FixItInsertionLine.begin() + HintCol);
+
+        PrevHintEndCol = LastColumnModified;
       } else {
         FixItInsertionLine.clear();
         break;

clang/test/FixIt/fixit-unicode.c

@@ -1,10 +1,11 @@
 // RUN: %clang_cc1 -fsyntax-only %s 2>&1 | FileCheck -strict-whitespace %s
-// PR13312
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck -check-prefix=CHECK-MACHINE %s
 
 struct Foo {
   int bar;
 };
 
+// PR13312
 void test1() {
   struct Foo foo;
   (&foo)☃>bar = 42;
@@ -12,4 +13,19 @@ void test1() {
 // Make sure we emit the fixit right in front of the snowman.
 // CHECK: {{^        \^}}
 // CHECK: {{^        ;}}
+
+// CHECK-MACHINE: fix-it:"{{.*}}fixit-unicode.c":{11:9-11:9}:";"
+}
+
+
+int printf(const char *, ...);
+void test2() {
+  printf("∆: %d", 1L);
+// CHECK: warning: format specifies type 'int' but the argument has type 'long'
+// Don't crash emitting a fixit after the delta.
+//     CHECK-NEXT:  printf("∆: %d", 1L);
+// CHECK-NEXT: {{^             ~~   \^~}}
+// CHECK-NEXT: {{^             %ld}}
+
+// CHECK-MACHINE: fix-it:"{{.*}}fixit-unicode.c":{23:16-23:18}:"%ld"
 }