[cmake] Disable C4129 warning for MSVC.

[fhahn]

Summary

This patch disables C4129 warnings ("'character' : unrecognized
character escape sequence"), because MSVC also warns about them in raw
string literals. This leads to spurious warnings for changes, e.g. for
the code introduced in D73078.

Unfortunately I do not have access to MSVC to test this change.

[dstuttard]

LGTM

Tested this in my environment and it fixed the issue.

[amccarth]

I'm reluctant to globally disable a useful warning given that the false positives problem has already been fixed for a while (since, at least, MSVC 2017).

In older versions of MSVC, I believe this will actually miscompile, so the "false positive" warning is actually useful.

The issue, as I understand it, comes in code like this:

  EXPECT_EQ(R"(digraph VPlan {
graph [labelloc=t, fontsize=30; label="Vectorization Plan"]
node [shape=rect, fontname=Courier, fontsize=30]
edge [fontname=Courier, fontsize=30]
compound=true
  N0 [label =
    ":\n" +
      "EMIT %vp0 = catchswitch\l" +
      "EMIT %vp1 = ret %vp0\l" +
      "EMIT %vp2 = br %vp0 %vp1\l"
  ]
  N0 -> N1 [ label=""]
  N1 [label =
    ":\n" +
      "EMIT %vp3 = indirectbr %vp2 %vp1\l" +
      "EMIT %vp4 = invoke %vp0\l"
  ]
}
)",
            FullDump);

The older MSVC preprocessor didn't tokenize raw string literals, so it treated the raw string as a big blob. This bug was fixed in MSVC 2017.

EXPECT_EQ is an assert-like macro that uses the preprocessor's # operator to stringify its arguments. Since the raw string hadn't yet been tokenized, the stringify effectively just put quotation marks around the literal representation of the raw string, creating a regular string literal that contains invalid junk like the unrecognized escape sequences and possibly problems with the internal quotation marks. In effect, this creates a miscompile because the message EXPECT_EQ would display upon failure would be garbled.

At a minimum, I'd like to see the comment changed to say "... because MSVC before 2017 warns ...".

A TODO would be nice so that we know to can re-enable the warning after LLVM progresses to MSVC 2017 as a baseline.

Another option would be to change the code to avoid using raw strings as arguments to macros that are going to be stringified. This means EXPECT_EQ failure messages _might_ be a tiny bit harder to understand.

[RKSimon]

LLVM's minimum VS version is already 2017+ and we're seeing this in 2017/2019 builds.

[amccarth]

Very strange. I have notes from when I tested this about a year and half ago. MSVC must have regressed. Bummer.

[rnk]

I messed around with this in godbolt and couldn't reproduce the warning with any version that I tried:
https://gcc.godbolt.org/z/7-c8oa
Not sure if my example works, though.

I couldn't get any old versions to miscompile as Adrian described either, but I didn't test all versions exhaustively.

[amccarth]

In D76428#1932174, @rnk wrote:

I messed around with this in godbolt and couldn't reproduce the warning with any version that I tried:
https://gcc.godbolt.org/z/7-c8oa
Not sure if my example works, though.

The key is that the raw string has to be an argument to a macro that then tries to stringify the argument.

The vectorizing tests mentioned in this thread are indeed generating the error with VS 2019.

I couldn't get any old versions to miscompile as Adrian described either, but I didn't test all versions exhaustively.

To see that, I think you have to go farther back than Compiler Explorer currently offers, like 19.0.

Given this information, I'm OK with this patch. In an ideal world, I'd hope for a more localized fix, but then the problem would probably crop up again and again.

[fhahn]

Thanks for all the input!

In D76428#1932354, @amccarth wrote:

In D76428#1932174, @rnk wrote:

I messed around with this in godbolt and couldn't reproduce the warning with any version that I tried:
https://gcc.godbolt.org/z/7-c8oa
Not sure if my example works, though.

The key is that the raw string has to be an argument to a macro that then tries to stringify the argument.

I could move the string to a variable outside of the macro, if that would fix the warning. Unfortunately I cannot confirm it fixes the warning on my setup.

[fhahn]

I've moved the expected raw string literals to variables in rG6120cb42f797. It looks like that was enough to silence the warning at least for the llvm-clang-x86_64-expensive-checks-win (http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/22891). Please let me know if there are any other configurations where this did not fix the warning.

llvm/cmake/modules/HandleLLVMOptions.cmake

set(msvc_warning_flags
           # C4592 is disabled because of false positives in Visual Studio 2015
           # Update 1. Re-evaluate the usefulness of this diagnostic with Update 2.
       -wd4592 # Suppress ''var': symbol will be dynamically initialized (implementation limitation)
       -wd4319 # Suppress ''operator' : zero extending 'type' to 'type' of greater size'
           # C4709 is disabled because of a bug with Visual Studio 2017 as of
           # v15.8.8. Re-evaluate the usefulness of this diagnostic when the bug
           # is fixed.
       -wd4709 # Suppress comma operator within array index expression
+      -wd4129 # Suppress ''character' : unrecognized character escape sequence'
+          # warning, because MSVC also warns about them in raw string  literals.
 
       # Ideally, we'd like this warning to be enabled, but even MSVC 2019 doesn't
       # support the 'aligned' attribute in the way that clang sources requires (for
       # any code that uses the LLVM_ALIGNAS macro), so this is must be disabled to
       # avoid unwanted alignment warnings.
       -wd4324 # Suppress 'structure was padded due to __declspec(align())'
 
       # Promoted warnings.