On Fri, Jul 20, 2012 at 9:39 AM, Jordan Rose <[email protected]> wrote: > Ouch, since the original bug is a crash as well.
Yeah :-/ PR13417 felt like a crash that's much more common in practice though. Thanks for investigating! > I'll take a look at this today (and PR13418 too). > > > On Jul 19, 2012, at 11:45 PM, Nico Weber wrote: > >> I reverted this change for now in r160542. >> >> On Thu, Jul 19, 2012 at 11:08 PM, Nico Weber <[email protected]> wrote: >>> Is it possible that this caused http://llvm.org/bugs/show_bug.cgi?id=13417 ? >>> >>> On Mon, Jul 16, 2012 at 1:52 PM, Jordan Rose <[email protected]> wrote: >>>> Author: jrose >>>> Date: Mon Jul 16 15:52:12 2012 >>>> New Revision: 160319 >>>> >>>> URL: http://llvm.org/viewvc/llvm-project?rev=160319&view=rev >>>> Log: >>>> 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> >>>> >>>> Modified: >>>> cfe/trunk/docs/UsersManual.html >>>> cfe/trunk/lib/Frontend/TextDiagnostic.cpp >>>> cfe/trunk/test/FixIt/fixit-unicode.c >>>> >>>> Modified: cfe/trunk/docs/UsersManual.html >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UsersManual.html?rev=160319&r1=160318&r2=160319&view=diff >>>> ============================================================================== >>>> --- cfe/trunk/docs/UsersManual.html (original) >>>> +++ cfe/trunk/docs/UsersManual.html Mon Jul 16 15:52:12 2012 >>>> @@ -229,6 +229,9 @@ >>>> >>>> <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 @@ >>>> </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 @@ >>>> "\\"), tabs (as "\t"), newlines (as "\n"), >>>> double >>>> quotes(as "\"") and non-printable characters (as octal >>>> "\xxx").</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"> >>>> >>>> Modified: cfe/trunk/lib/Frontend/TextDiagnostic.cpp >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/TextDiagnostic.cpp?rev=160319&r1=160318&r2=160319&view=diff >>>> ============================================================================== >>>> --- cfe/trunk/lib/Frontend/TextDiagnostic.cpp (original) >>>> +++ cfe/trunk/lib/Frontend/TextDiagnostic.cpp Mon Jul 16 15:52:12 2012 >>>> @@ -1124,7 +1124,7 @@ >>>> 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 @@ >>>> 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 @@ >>>> // >>>> // 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; >>>> - >>>> - // 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); >>>> - } >>>> + if (HintCol < PrevHintEndCol) >>>> + HintCol = PrevHintEndCol + 1; >>>> >>>> + // 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); >>>> + FixItInsertionLine.begin() + HintCol); >>>> >>>> - PrevHintEnd = LastColumnModified; >>>> + PrevHintEndCol = LastColumnModified; >>>> } else { >>>> FixItInsertionLine.clear(); >>>> break; >>>> >>>> Modified: cfe/trunk/test/FixIt/fixit-unicode.c >>>> URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/FixIt/fixit-unicode.c?rev=160319&r1=160318&r2=160319&view=diff >>>> ============================================================================== >>>> --- cfe/trunk/test/FixIt/fixit-unicode.c (original) >>>> +++ cfe/trunk/test/FixIt/fixit-unicode.c Mon Jul 16 15:52:12 2012 >>>> @@ -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 @@ >>>> // 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" >>>> } >>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> [email protected] >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
