This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Disable C4129 warning for MSVC.
AbandonedPublic

Authored by fhahn on Mar 19 2020, 6:15 AM.

Details

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.

Diff Detail

Event Timeline

fhahn created this revision.Mar 19 2020, 6:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 19 2020, 6:15 AM
Herald added a subscriber: mgorny. · View Herald Transcript
dstuttard accepted this revision.Mar 19 2020, 7:33 AM

LGTM

Tested this in my environment and it fixed the issue.

This revision is now accepted and ready to land.Mar 19 2020, 7:33 AM

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.

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

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

rnk added a comment.Mar 19 2020, 2:21 PM

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.

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.

Thanks for all the input!

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 abandoned this revision.Mar 31 2020, 6:03 AM

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.