[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.

[aaron.ballman]

clang/include/clang/Basic/DiagnosticLexKinds.td

116–117

What would it take to enable the diagnostic by default? We don't typically add off-by-default diagnostics because there's evidence that not many folks enable them.

Alternatively, would this make sense as a pedantic warning? It's not really an extension for us to accept this stuff, but it seems like we can still pedantically diagnose the code?

clang/lib/Lex/Lexer.cpp

2400

It looks like this can move into the while loop and we can remove some = false from within the loop?

2408

Do you have some idea of performance cost for enabling the warning with a large TU? Are we talking .5%? 1%? 5%? noise?

2416

So if unicode decoding didn't fail... diagnose? :-) The naming here seems unfortunate.

2416–2418

2696

I think this can move into the loop as well.

2806–2808

Same concerns about naming here as above.

llvm/lib/Support/ConvertUTF.cpp

426–429

[cor3ntin]

• Address style comments
• Improve commit message
• Enable the warning in -pedantic

[cor3ntin]

clang/lib/Lex/Lexer.cpp

2400

The idea here is to not diagnose a contiguous sequence of invalid code units more than once
The sequence 0x80x0x80x80 for example will only warn one for example. The goal is to avoid noise when, for example there is a long comment in Windows-1251 encoded cyrillic. it would warn one per word (because the spaces would decode fine) rather than one per contiguous non-sense character.
It's also somewhat necessary because we can't know where the end of the invalid "character" is, better to be greedy.
(The test file check that behavior)

2408

This adds 2 comparisons when the warning is enbled (is ASCII(c) is simply C < 0x80) - so it should be noise, but I have not run benchmark.
It might be more noticeable within multi line comments as there the optimization that vectorizes the comment skipping on SSE2 platforms is simply skipped when this optimization is enabled.

2416

if(!UnicodeDecodeFailedInThePreviousLoopIteration) ? I'm open to suggestion here

[aaron.ballman]

clang/lib/Lex/Lexer.cpp

2400

Oh derp, you're right (of course), sorry for the noise!

2408

Ah, no need to run the benchmark then, I was thinking this would involve function call overhead and more complex checking for the isASCII call than exists in reality.

2416

UnicodeDecodingAlreadyDiagnosed

2701–2702

However, *this* seems like it could noticeably impact performance, especially for C code (including C header files in C++ mode), and may be worth getting measurements for.

[cor3ntin]

s/UnicodeDecodeFailed/UnicodeDecodingAlreadyDiagnosed/

[cor3ntin]

Make sure the warning is off by default.

[tahonermann]

clang/include/clang/Basic/DiagnosticLexKinds.td

116–117

I agree with enabling this by default in at least some warning mode. Aaron's suggestion of making it a pedantic warning seems reasonable to me.

clang/lib/Lex/Lexer.cpp

2406–2425

I think it would be helpful to include a link to Unicode PR issue 121 here and note that the diagnostic follows policy option 1. Likewise for handling of '//' comment styles below. Alternatively, provide the link and a note in the release notes.

[cor3ntin]

Vectorizes comment skipping even when -Winvalid-utf8 is enabled.

[cor3ntin]

Further optimize

[cor3ntin]

@aaron.ballman

I created a 1.3GB file containing 500000 comments of random size and content,
with the following script

import string
import random

for i in range(0, 500000):
    print("/*{}*/".format(''.join(random.choices('\n' + string.ascii_uppercase + string.digits,
                            k=random.randrange(400, 5000)))
    ))
print("int main() {}")

The results are as follow:

Benchmark 1: /home/cor3ntin/dev/compilers/LLVM/build_release_main/bin/clang -Isqlite huge.cpp
  Time (mean ± σ):     221.3 ms ±   4.0 ms    [User: 151.5 ms, System: 69.7 ms]
  Range (min … max):   217.2 ms … 229.6 ms    13 runs
 
Benchmark 2: bin/clang -Isqlite huge.cpp
  Time (mean ± σ):     212.7 ms ±   7.6 ms    [User: 155.2 ms, System: 57.2 ms]
  Range (min … max):   204.6 ms … 226.9 ms    14 runs
 
Benchmark 3: ./bin/clang -Winvalid-utf8 -Isqlite huge.cpp
  Time (mean ± σ):     210.9 ms ±   4.0 ms    [User: 151.8 ms, System: 58.9 ms]
  Range (min … max):   205.7 ms … 220.2 ms    14 runs
 
Summary
  './bin/clang -Winvalid-utf8 -Isqlite huge.cpp' ran
    1.01 ± 0.04 times faster than 'bin/clang -Isqlite huge.cpp'
    1.05 ± 0.03 times faster than '/home/cor3ntin/dev/compilers/LLVM/build_release_main/bin/clang -Isqlite huge.cpp

I also tried sqlite (8MB file with all the sources)

Benchmark 1: /home/cor3ntin/dev/compilers/LLVM/build_release_main/bin/clang -Isqlite sqlite/sqlite3.c
  Time (mean ± σ):      1.852 s ±  0.033 s    [User: 1.782 s, System: 0.069 s]
  Range (min … max):    1.806 s …  1.904 s    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark 2: bin/clang -Isqlite sqlite/sqlite3.c
  Time (mean ± σ):      1.758 s ±  0.010 s    [User: 1.694 s, System: 0.061 s]
  Range (min … max):    1.738 s …  1.774 s    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark 3: ./bin/clang -Winvalid-utf8 -Isqlite sqlite/sqlite3.c
  Time (mean ± σ):      1.756 s ±  0.021 s    [User: 1.700 s, System: 0.053 s]
  Range (min … max):    1.741 s …  1.810 s    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  './bin/clang -Winvalid-utf8 -Isqlite sqlite/sqlite3.c' ran
    1.00 ± 0.01 times faster than 'bin/clang -Isqlite sqlite/sqlite3.c'
    1.05 ± 0.02 times faster than '/home/cor3ntin/dev/compilers/LLVM/build_release_main/bin/clang -Isqlite sqlite/sqlite3.c'

and a file including all the c++ standard headers

Benchmark 1: /home/cor3ntin/dev/compilers/LLVM/build_release_main/bin/clang -Isqlite headers.cpp
  Time (mean ± σ):     956.6 ms ±  12.9 ms    [User: 891.1 ms, System: 63.0 ms]
  Range (min … max):   935.8 ms … 979.5 ms    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark 2: bin/clang -Isqlite headers.cpp
  Time (mean ± σ):     906.1 ms ±  19.4 ms    [User: 860.1 ms, System: 43.6 ms]
  Range (min … max):   883.8 ms … 941.3 ms    10 runs
 
  Warning: Ignoring non-zero exit code.
  Warning: The first benchmarking run for this command was significantly slower than the rest (941.3 ms). This could be caused by (filesystem) caches that were not filled until after the first run. You should consider using the '--warmup' option to fill those caches before the actual benchmark. Alternatively, use the '--prepare' option to clear the caches before each timing run.
 
Benchmark 3: ./bin/clang -Winvalid-utf8 -Isqlite headers.cpp
  Time (mean ± σ):     906.8 ms ±  16.1 ms    [User: 866.5 ms, System: 39.5 ms]
  Range (min … max):   887.6 ms … 940.6 ms    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  'bin/clang -Isqlite headers.cpp' ran
    1.00 ± 0.03 times faster than './bin/clang -Winvalid-utf8 -Isqlite headers.cpp'
    1.06 ± 0.03 times faster than '/home/cor3ntin/dev/compilers/LLVM/build_release_main/bin/clang -Isqlite headers.cpp'

You will notice i no longer check for whether the diagnostic is enabled preemptively as this turned out to be have negative effects on performance.

I posted bogus results earlier because i was using a pre built clang as the baseline and i suppose it's more optimized than the default build options, or was compiled with a different compiler

[aaron.ballman]

It looks like precommit CI caught some issues:

Failed Tests (2):
  Clangd :: utf8.test
  Clangd Unit Tests :: ./ClangdTests.exe/CompletionStringTest/GetDeclCommentBadUTF8

clang/include/clang/Basic/DiagnosticLexKinds.td

116–117

Great, now that it's marked Extension it will show up when someone enables -pedantic (but is otherwise off by default).

clang/lib/Lex/Lexer.cpp

2406–2425

+1 -- I hadn't realized there were other options. However, ultimately I think this should be committed when WG21 has adopted the paper and then it would be even better to have a comment citing which stable name and paragraph number the code is implementing from P2295.

[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!

[marginstrawberries]

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 free games

Good point. The error was confusing, but it's probably a segfault while running.

[tahonermann]

@cor3ntin, sorry for failing to keep up with reviews; I know this has already been committed. I did spot a couple of typos should you feel inclined to address them.

clang/lib/Lex/Lexer.cpp

2399

2695

[cfe-commits]

I'll be unavailable the next 2 weeks, feel free to do it if you want!

[Fako432]

In D128059#3640564, @cor3ntin wrote:

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/monkey mart

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.

You raise a valid argument. The confusion caused by the error could potentially be attributed to a segmentation fault occurring during the execution.

clang/docs/ReleaseNotes.rst

- The ``-Wdeprecated`` diagnostic will now warn on out-of-line ``constexpr``
   declarations downgraded to definitions in C++1z, in addition to the
   existing warning on out-of-line ``const`` declarations.
 - ``-Wshift-overflow`` will not warn for signed left shifts in C++20 mode
   (and newer), as it will always wrap and never overflow. This fixes
   `Issue 52873 <https://github.com/llvm/llvm-project/issues/52873>`_.
 - When using class templates without arguments, clang now tells developers
   that template arguments are missing in certain contexts.
   This fixes `Issue 55962 <https://github.com/llvm/llvm-project/issues/55962>`_.
 - Printable Unicode characters within `static_assert` messages are no longer
   escaped.
 - The ``-Winfinite-recursion`` diagnostic no longer warns about
   unevaluated operands of a ``typeid`` expression, as they are now
   modeled correctly in the CFG. This fixes
   `Issue 21668 <https://github.com/llvm/llvm-project/issues/21668>`_.
-- ``-Wself-assign``, ``-Wself-assign-overloaded`` and ``-Wself-move`` will 
+- ``-Wself-assign``, ``-Wself-assign-overloaded`` and ``-Wself-move`` will
   suggest a fix if the decl being assigned is a parameter that shadows a data
   member of the contained class.
+- Added ``-Winvalid-utf8`` which diagnoses invalid UTF-8 code unit sequences in
+  comments.
 
 Non-comprehensive list of changes in this release
 -------------------------------------------------
 
 - Improve __builtin_dump_struct:
 
   - Support bitfields in struct and union.
   - Improve the dump format, dump both bitwidth(if its a bitfield) and field

 
 AST Matchers
 ------------
 
 - Expanded ``isInline`` narrowing matcher to support C++17 inline variables.
 
 - Added ``forEachTemplateArgument`` matcher which creates a match every
   time a ``templateArgument`` matches the matcher supplied to it.
-  
 - Added ``objcStringLiteral`` matcher which matches ObjectiveC String
   literal expressions.
 
 clang-format
 ------------
 
 - **Important change**: Renamed ``IndentRequires`` to ``IndentRequiresClause``
   and changed the default for all styles from ``false`` to ``true``.

clang/include/clang/Basic/DiagnosticLexKinds.td

def warn_multichar_character_literal : Warning<
   "multi-character character constant">, InGroup<MultiChar>;
 def warn_four_char_character_literal : Warning<
   "multi-character character constant">, InGroup<FourByteMultiChar>, DefaultIgnore;
 
 
 // Unicode and UCNs
 def err_invalid_utf8 : Error<
   "source file is not valid UTF-8">;
+def warn_invalid_utf8_in_comment : Extension<
+  "invalid UTF-8 in comment">, InGroup<DiagGroup<"invalid-utf8">>;
 def err_character_not_allowed : Error<
   "unexpected character <U+%0>">;
 def err_character_not_allowed_identifier : Error<
   "character <U+%0> not allowed %select{in|at the start of}1 an identifier">;
 def ext_unicode_whitespace : ExtWarn<
   "treating Unicode character as whitespace">,
   InGroup<DiagGroup<"unicode-whitespace">>;
 def warn_utf8_symbol_homoglyph : Warning<

clang/lib/Lex/Lexer.cpp

bool Lexer::SkipLineComment(Token &Result, const char *CurPtr,
   }
 
   // Scan over the body of the comment.  The common case, when scanning, is that
   // the comment contains normal ascii characters with nothing interesting in
   // them.  As such, optimize for this case with the inner loop.
   //
   // This loop terminates with CurPtr pointing at the newline (or end of buffer)
   // character that ends the line comment.
+
+  // C++23 [lex.phases] p1
+  // Diagnose invalid UTF-8 if the corresponding warning is enabled, emitting a
+  // diagnostic only once per entire ill-formed subsequence to avoid
+  // emiting to many diagnostics (see http://unicode.org/review/pr-121.html).
+  bool UnicodeDecodingAlreadyDiagnosed = false;
+
   char C;
   while (true) {
     C = *CurPtr;
     // Skip over characters in the fast loop.
-    while (C != 0 &&                // Potentially EOF.
-           C != '\n' && C != '\r')  // Newline or DOS-style newline.
+    while (isASCII(C) && C != 0 &&   // Potentially EOF.
+           C != '\n' && C != '\r') { // Newline or DOS-style newline.
       C = *++CurPtr;
+      UnicodeDecodingAlreadyDiagnosed = false;
+    }
+
+    if (!isASCII(C)) {
+      unsigned Length = llvm::getUTF8SequenceSize(
+          (const llvm::UTF8 *)CurPtr, (const llvm::UTF8 *)BufferEnd);
+      if (Length == 0) {
+        if (!UnicodeDecodingAlreadyDiagnosed && !isLexingRawMode())
+          Diag(CurPtr, diag::warn_invalid_utf8_in_comment);
+        UnicodeDecodingAlreadyDiagnosed = true;
+        ++CurPtr;
+      } else {
+        UnicodeDecodingAlreadyDiagnosed = false;
+        CurPtr += Length;
+      }
+      continue;
+    }
 
     const char *NextLine = CurPtr;
     if (C != 0) {
       // We found a newline, see if it's escaped.
       const char *EscapePtr = CurPtr-1;
       bool HasSpace = false;
       while (isHorizontalWhitespace(*EscapePtr)) { // Skip whitespace.
         --EscapePtr;
if (C == 0 && CurPtr == BufferEnd+1) {
     return false;
   }
 
   // Check to see if the first character after the '/*' is another /.  If so,
   // then this slash does not end the block comment, it is part of it.
   if (C == '/')
     C = *CurPtr++;
 
+  // C++23 [lex.phases] p1
+  // Diagnose invalid UTF-8 if the corresponding warning is enabled, emitting a
+  // diagnostic only once per entire ill-formed subsequence to avoid
+  // emiting to many diagnostics (see http://unicode.org/review/pr-121.html).
+  bool UnicodeDecodingAlreadyDiagnosed = false;
+
   while (true) {
     // Skip over all non-interesting characters until we find end of buffer or a
     // (probably ending) '/' character.
     if (CurPtr + 24 < BufferEnd &&
         // If there is a code-completion point avoid the fast scan because it
         // doesn't check for '\0'.
         !(PP && PP->getCodeCompletionFileLoc() == FileLoc)) {
       // While not aligned to a 16-byte boundary.
-      while (C != '/' && ((intptr_t)CurPtr & 0x0F) != 0)
+      while (C != '/' && (intptr_t)CurPtr % 16 != 0) {
+        if (!isASCII(C))
+          goto MultiByteUTF8;
         C = *CurPtr++;
-
+      }
       if (C == '/') goto FoundSlash;
 
 #ifdef __SSE2__
       __m128i Slashes = _mm_set1_epi8('/');
-      while (CurPtr+16 <= BufferEnd) {
+      while (CurPtr + 16 < BufferEnd) {
+        int Mask = _mm_movemask_epi8(*(const __m128i *)CurPtr);
+        if (LLVM_UNLIKELY(Mask != 0)) {
+          goto MultiByteUTF8;
+        }
+        // look for slashes
         int cmp = _mm_movemask_epi8(_mm_cmpeq_epi8(*(const __m128i*)CurPtr,
                                     Slashes));
         if (cmp != 0) {
           // Adjust the pointer to point directly after the first slash. It's
           // not necessary to set C here, it will be overwritten at the end of
           // the outer loop.
           CurPtr += llvm::countTrailingZeros<unsigned>(cmp) + 1;
           goto FoundSlash;
         }
         CurPtr += 16;
       }
 #elif __ALTIVEC__
+      __vector unsigned char LongUTF = {0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
+                                        0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
+                                        0x80, 0x80, 0x80, 0x80};
       __vector unsigned char Slashes = {
         '/', '/', '/', '/',  '/', '/', '/', '/',
         '/', '/', '/', '/',  '/', '/', '/', '/'
       };
-      while (CurPtr + 16 <= BufferEnd &&
-             !vec_any_eq(*(const __vector unsigned char *)CurPtr, Slashes))
+      while (CurPtr + 16 < BufferEnd) {
+        if (LLVM_UNLIKELY(
+                vec_any_ge(*(const __vector unsigned char *)CurPtr, LongUTF)))
+          goto MultiByteUTF8;
+        if (vec_any_eq(*(const __vector unsigned char *)CurPtr, Slashes)) {
+          break;
+        }
         CurPtr += 16;
+      }
+
 #else
-      // Scan for '/' quickly.  Many block comments are very large.
-      while (CurPtr[0] != '/' &&
-             CurPtr[1] != '/' &&
-             CurPtr[2] != '/' &&
-             CurPtr[3] != '/' &&
-             CurPtr+4 < BufferEnd) {
-        CurPtr += 4;
+      while (CurPtr + 16 < BufferEnd) {
+        bool HasNonASCII = false;
+        for (unsigned I = 0; I < 16; ++I)
+          HasNonASCII |= !isASCII(CurPtr[I]);
+
+        if (LLVM_UNLIKELY(HasNonASCII))
+          goto MultiByteUTF8;
+
+        bool HasSlash = false;
+        for (unsigned I = 0; I < 16; ++I)
+          HasSlash |= CurPtr[I] == '/';
+        if (HasSlash)
+          break;
+        CurPtr += 16;
       }
 #endif
 
       // It has to be one of the bytes scanned, increment to it and read one.
       C = *CurPtr++;
     }
 
-    // Loop to scan the remainder.
-    while (C != '/' && C != '\0')
+    // Loop to scan the remainder, warning on invalid UTF-8
+    // if the corresponding warning is enabled, emitting a diagnostic only once
+    // per sequence that cannot be decoded.
+    while (C != '/' && C != '\0') {
+      if (isASCII(C)) {
+        UnicodeDecodingAlreadyDiagnosed = false;
         C = *CurPtr++;
+        continue;
+      }
+    MultiByteUTF8:
+      // CurPtr is 1 code unit past C, so to decode
+      // the codepoint, we need to read from the previous position.
+      unsigned Length = llvm::getUTF8SequenceSize(
+          (const llvm::UTF8 *)CurPtr - 1, (const llvm::UTF8 *)BufferEnd);
+      if (Length == 0) {
+        if (!UnicodeDecodingAlreadyDiagnosed && !isLexingRawMode())
+          Diag(CurPtr - 1, diag::warn_invalid_utf8_in_comment);
+        UnicodeDecodingAlreadyDiagnosed = true;
+      } else {
+        UnicodeDecodingAlreadyDiagnosed = false;
+        CurPtr += Length - 1;
+      }
+      C = *CurPtr++;
+    }
 
     if (C == '/') {
   FoundSlash:
       if (CurPtr[-2] == '*')  // We found the final */.  We're done!
         break;
 
       if ((CurPtr[-2] == '\n' || CurPtr[-2] == '\r')) {
         if (isEndOfBlockCommentWithEscapedNewLine(CurPtr - 2, this,
                                                   LangOpts.Trigraphs)) {
           // We found the final */, though it had an escaped newline between the
           // * and /.  We're done!
           break;
         }
       }
       if (CurPtr[0] == '*' && CurPtr[1] != '/') {
         // If this is a /* inside of the comment, emit a warning.  Don't do this
         // if this is a /*/, which will end the comment.  This misses cases with
         // embedded escaped newlines, but oh well.
         if (!isLexingRawMode())
           Diag(CurPtr-1, diag::warn_nested_block_comment);
       }

clang/test/Lexer/comment-invalid-utf8.c

+// RUN: %clang_cc1 -fsyntax-only %s -Winvalid-utf8 -verify=expected
+// RUN: %clang_cc1 -fsyntax-only %s -verify=nowarn
+// nowarn-no-diagnostics
+
+// This file is purposefully encoded as windows-1252
+// be careful when modifying.
+
+//€
+// expected-warning@-1 {{invalid UTF-8 in comment}}
+
+// € ‚ƒ„…†‡ˆ‰ Š ‹ Œ Ž
+// expected-warning@-1 6{{invalid UTF-8 in comment}}
+
+/*€*/
+// expected-warning@-1 {{invalid UTF-8 in comment}}
+
+/*€ ‚ƒ„…†‡ˆ‰ Š ‹ Œ Ž*/
+// expected-warning@-1 6{{invalid UTF-8 in comment}}
+
+/*
+€
+*/
+// expected-warning@-2 {{invalid UTF-8 in comment}}
+
+// abcd
+// €abcd
+// expected-warning@-1 {{invalid UTF-8 in comment}}

clang/test/Lexer/comment-utf8.c

+// RUN: %clang_cc1 -fsyntax-only %s -Winvalid-utf8 -verify
+// expected-no-diagnostics
+
+
+//§ § § 😀 你好 ©
+
+/*§ § § 😀 你好 ©*/
+
+/*
+§ § § 😀 你好 ©©©
+*/
+
+/* § § § 😀 你好 © */
+/*
+    a longer comment to exerce the vectorized code path
+    ----------------------------------------------------
+    αααααααααααααααααααααα      // here is some unicode
+    ----------------------------------------------------
+    ----------------------------------------------------
+*/
+
+// The following test checks that a short comment is not merged
+// with a subsequent long comment containing utf-8
+enum a {
+    x  /* 01234567890ABCDEF*/
+};
+/*ααααααααα*/

clang/test/SemaCXX/static-assert.cpp

-// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -pedantic -triple=x86_64-linux-gnu
+// RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11 -pedantic -triple=x86_64-linux-gnu -Wno-invalid-utf8
 
 int f(); // expected-note {{declared here}}
 
 static_assert(f(), "f"); // expected-error {{static_assert expression is not an integral constant expression}} expected-note {{non-constexpr function 'f' cannot be used in a constant expression}}
 static_assert(true, "true is not false");
 static_assert(false, "false is false"); // expected-error {{static_assert failed: false is false}}
 
 void g() {

llvm/include/llvm/Support/ConvertUTF.h

 ConversionResult ConvertUTF32toUTF16 (
   const UTF32** sourceStart, const UTF32* sourceEnd,
   UTF16** targetStart, UTF16* targetEnd, ConversionFlags flags);
 
 Boolean isLegalUTF8Sequence(const UTF8 *source, const UTF8 *sourceEnd);
 
 Boolean isLegalUTF8String(const UTF8 **source, const UTF8 *sourceEnd);
 
+unsigned getUTF8SequenceSize(const UTF8 *source, const UTF8 *sourceEnd);
+
 unsigned getNumBytesForUTF8(UTF8 firstByte);
 
 /*************************************************************************/
 /* Below are LLVM-specific wrappers of the functions above. */
 
 template <typename T> class ArrayRef;
 template <typename T> class SmallVectorImpl;
 class StringRef;

llvm/lib/Support/ConvertUTF.cpp

 Boolean isLegalUTF8Sequence(const UTF8 *source, const UTF8 *sourceEnd) {
     int length = trailingBytesForUTF8[*source]+1;
     if (length > sourceEnd - source) {
         return false;
     }
     return isLegalUTF8(source, length);
 }
 
+/*
+ * Exported function to return the size of the first utf-8 code unit sequence,
+ * Or 0 if the sequence is not valid;
+ */
+unsigned getUTF8SequenceSize(const UTF8 *source, const UTF8 *sourceEnd) {
+  int length = trailingBytesForUTF8[*source] + 1;
+  return (length <= sourceEnd - source && isLegalUTF8(source, length)) ? length
+                                                                       : 0;
+}
+
 /* --------------------------------------------------------------------- */
 
 static unsigned
 findMaximalSubpartOfIllFormedUTF8Sequence(const UTF8 *source,
                                           const UTF8 *sourceEnd) {
   UTF8 b1, b2, b3;
 
   assert(!isLegalUTF8Sequence(source, sourceEnd));