[Clang] Add a warning on invalid UTF-8 in comments.

[cor3ntin]

Summary

Introduce an off-by default -Winvalid-utf8 warning
that detects invalid UTF-8 code units sequences in comments.

Invalid UTF-8 in other places is already diagnosed,
as that cannot appear in identifiers and other grammar constructs.

The warning is off by default as its likely to be somewhat disruptive
otherwise.

This warning allows clang to conform to the yet-to be approved WG21
"P2295R5 Support for UTF-8 as a portable source file encoding"
paper.

[cor3ntin]

clang/lib/Lex/Lexer.cpp

2406–2425

I don't think this applies as we are not inserting replacement characters, but I'm all for adding a comment.
I don't know if we should reference the c++ wording though, as I don;t think there is value in not applying that behavior in all language modes.

2718

This crashes when using _mm_load_si128 which suprises me because CurPtr is supposedly aligned on a 16 bytes boundary here. Any idea?

[tahonermann]

clang/lib/Lex/Lexer.cpp

2406–2425

Perhaps phrase it something like "diagnostics are issued consistent with the replacement character insertion strategy of Unicode PR-121 policy option 1". The goal is to make it clear that the diagnostic strategy is not out of thin air; that it follows Unicode endorsed behavior.

[aaron.ballman]

clang/lib/Lex/Lexer.cpp

2406–2425

Tom covered why I would prefer to see a comment, but as for the standards reference -- we already put in references to only one language for behavior we treat as an extension, so this would be more of the same. That ends up being helpful (especially if there's an explicit comment about it being an extension in other languages) because it tells the reader how the extension is roughly expected to behave as well.

2718

Wait, did you verify that CurPtr really is on a 16-byte boundary, or are you thinking it should be on such a boundary? (I don't see any alignment markings on the parameter, so I'd assume it's aligned as any other pointer.)

[aaron.ballman]

clang/lib/Lex/Lexer.cpp

2718

Derp, I missed that the loop above is manually aligning the pointer.

I'm not certain what's going on here with your crash...

[cor3ntin]

clang/lib/Lex/Lexer.cpp

2718

Just above, i do

while (C != '/' && ((intptr_t)CurPtr & 0x0F) != 0) {
        if (!isASCII(C)) {
          CurPtr--;
          break;
        }

Derp indeed (the CurPtr--; break is the culprit, fix shortly)

[cor3ntin]

• Add a comment referencing the (to be) C++ wording and unicode discussion on replacemet characters
• Do not try to skip utf8 checks when the warning is not enabled as checking whether the warning is enabled is more expensive
• Fix a bug causing vectorization to not be aligned

[cor3ntin]

• Remove explicit load instruction
• cleanup extra braces

[cor3ntin]

@aaron.ballman FYI this is now core-approved

[erichkeane]

clang/docs/ReleaseNotes.rst

281

[cor3ntin]

Fix typo.

[aaron.ballman]

clang/docs/ReleaseNotes.rst

282

Should we mention P2295R5 now that it's at least core approved? Something like:

...unit sequences in comments; in support of `P2295R5 <http://wg21.link/P2295R5>`_.

[cor3ntin]

My plan is to do that for all papers once past plenary to
keep consistency with cxx_status.

[aaron.ballman]

The changes here LGTM, thank you for this!

[thakis]

As far as I can tell, this breaks check-clang everywhere: http://45.33.8.238/

Please take a look and revert for now if it takes a while to fix.

[cor3ntin]

In D128059#3633343, @thakis wrote:

As far as I can tell, this breaks check-clang everywhere: http://45.33.8.238/

Please take a look and revert for now if it takes a while to fix.

Thanks for letting me know, i hadn't noticed the bots screaming at me. I did revert until i can investigate and fix.

[cor3ntin]

Fix the test of a newly landed commit

[cor3ntin]

The bots found a further issue which i committed a fix for directly https://reviews.llvm.org/D129223 - I should have caught that sooner

[cor3ntin]

Deploying that turned out to reveal a few critical issues

• getUTF8SequenceSize never reported a non-zero length for valid

UTF-8 sequences.

• In *some* circumstances (depending on the size of comment),

Unicode codepoints were parsed from one past their start,
because the CurPtr was sometimes, but not always, moved back.

I also added a test file with *valid* utf-8 in comments
(which would have caught these issues).

[cor3ntin]

@aaron.ballman Ok, this didn't turn out great, and given the changes + the fact it was completely broken (twice!), I'd love if you could take a look at it again. Thanks :)

[cor3ntin]

Fix bound check in the non vectorized case

[aaron.ballman]

I only spotted one thing I think is actually an issue, the rest is style related. LGTM with the one issue fixed.

clang/lib/Lex/Lexer.cpp

2707–2709

2751–2764

< instead of <=?

2753–2755

2761–2763

[cor3ntin]

Remove superfuous braces.

[cor3ntin]

@aaron.ballman Thanks for the review. I landed the changes and got a bunch of bots screaming at me for changes that are completely unrelated

https://lab.llvm.org/buildbot/#/builders/21/builds/45146
https://lab.llvm.org/buildbot/#/builders/36/builds/22838
https://lab.llvm.org/buildbot/#/builders/121/builds/21183

[hubert.reinterpretcast]

In D128059#3640424, @cor3ntin wrote:

@aaron.ballman Thanks for the review. I landed the changes and got a bunch of bots screaming at me for changes that are completely unrelated

https://lab.llvm.org/buildbot/#/builders/21/builds/45146
https://lab.llvm.org/buildbot/#/builders/36/builds/22838
https://lab.llvm.org/buildbot/#/builders/121/builds/21183

What do you mean it is completely unrelated? You have __ALTIVEC__-specific changes and it knocks out the PPC bots.

[cor3ntin]

In D128059#3640544, @hubert.reinterpretcast wrote:

In D128059#3640424, @cor3ntin wrote:

@aaron.ballman Thanks for the review. I landed the changes and got a bunch of bots screaming at me for changes that are completely unrelated

https://lab.llvm.org/buildbot/#/builders/21/builds/45146
https://lab.llvm.org/buildbot/#/builders/36/builds/22838
https://lab.llvm.org/buildbot/#/builders/121/builds/21183

What do you mean it is completely unrelated? You have __ALTIVEC__-specific changes and it knocks out the PPC bots.

Good point. The error was a bit misleading but i guess what's happening is a segfault when running clang-ast-dump.
I'm reverting for now and I don't really know how to go from there as I don't have the hardware to test that code path.

[hubert.reinterpretcast]

In D128059#3640564, @cor3ntin wrote:

Good point. The error was a bit misleading but i guess what's happening is a segfault when running clang-ast-dump.
I'm reverting for now and I don't really know how to go from there as I don't have the hardware to test that code path.

Sending you info via e-mail on how to request access to systems.

[cor3ntin]

Fix crash on PowerPC

(I forgot to removed a non-sense line that
could cause the CurPtr to move incorrectly
past a / in the ALTIVEC code path).

[aaron.ballman]

In D128059#3643081, @cor3ntin wrote:

Fix crash on PowerPC

(I forgot to removed a non-sense line that
could cause the CurPtr to move incorrectly
past a / in the ALTIVEC code path).

Ah, nice catch -- LGTM!

[JDevlieghere]

I had to revert this because it breaks a bunch of LLDB tests: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45288/#showFailuresLink. It looks like this causes an error when building some SDK modules.

[cor3ntin]

In D128059#3646695, @JDevlieghere wrote:

I had to revert this because it breaks a bunch of LLDB tests: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45288/#showFailuresLink. It looks like this causes an error when building some SDK modules.

Thanks (and sorry for the annoyance)

[cor3ntin]

This turned out to be an interesting bug.
The SSE code tried to be clever and skip over valid ascii code units when finding invalid UTF-8.
In doing so, it could run over the end of a comment entirely if

• there was a short ascii comment
• followed by a tiny amount of C++
• followed by another comment containing non-ascii data.

It does not matter whether it was valid or not (which was misleading
as the file that tripped the bot is full of invalid code units.

The problematic test boils down to

enum a {
    x  /* 01234567890ABCDEF*/
};
/*ααααααααα*/

The fix is to do in the SSE codepath what we do in the altivec
and default paths: if we find an invalid code unit,
we rescan that bit of the comment on the slow path
without trying to update CurPtr (and for each code unit,
checking both for isASCIII an != '/' at the same time.

[JDevlieghere]

In D128059#3646953, @cor3ntin wrote:

In D128059#3646695, @JDevlieghere wrote:

I had to revert this because it breaks a bunch of LLDB tests: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45288/#showFailuresLink. It looks like this causes an error when building some SDK modules.

Thanks (and sorry for the annoyance)

No worries at all. Thanks for looking into the issue!