[cfe-commits] r160319 - in /cfe/trunk: docs/UsersManual.html lib/Frontend/TextDiagnostic.cpp test/FixIt/fixit-unicode.c
Nico Weber
thakis at chromium.org
Fri Jul 20 09:42:52 PDT 2012
On Fri, Jul 20, 2012 at 9:39 AM, Jordan Rose <jordan_rose at apple.com> 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 <thakis at chromium.org> 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 <jordan_rose at apple.com> 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
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
More information about the cfe-commits
mailing list