RFR: 8263028: Windows build fails due to several treat-warning-as-errors

[Jorn Vernee]

On Tue, 23 Mar 2021 04:38:59 GMT, Yi Yang <yyang at openjdk.org> wrote:

>>> > The problem in methodMatcher.cpp is caused by this:
>>> > ```
>>> > #define RANGEBASE "\x1\x2\x3\x4\x5\x6\x7\x8\xa\xb\xc\xd\xe\xf" \
>>> >     "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f" \
>>> >     "\x21\x22\x23\x24\x25\x26\x27\x2a\x2b\x2c\x2d"  .....
>>> > ```
>>> > 
>>> > 
>>> > where the literal `\x25` is the `%` character. It seems like VC++ tries to interpret the `%` character even when it's inside square brackets, like
>>> > ```
>>> >       if (1 == sscanf(line, "%1022[[);/" RANGEBASE "]%n", sig+1, &bytes_read)) {
>>> > -> 
>>> >       if (1 == sscanf(line, "%1022[[);.....#$%&.....]%n", sig+1, &bytes_read)) {
>>> >                                              ^
>>> > ```
>>> > 
>>> > 
>>> > The [C++ reference](https://en.cppreference.com/w/c/io/fscanf) is unclear about how characters like `%` can be escaped inside square brackets (or whether they should be escaped at all).
>>> > Trying to use sscanf for this purpose makes the code hard to understand and non portable. It's better to ditch sscanf and read the characters byte-by-byte. That way, you can get rid of the original `PRAGMA_DISABLE_MSVC_WARNING(4819)` as well.
>>> 
>>> The explanation makes sense. We can parse class name and method name via byte-by-byte stream instead of advanced regex-like sscanf. For this reason, I also put a FIXME comment above MethodMatcher::parse_method_pattern.
>>> 
>>> This build failure also appears in the [downstream JDK](https://github.com/alibaba/dragonwell11/issues/70), blocking further development. So the purpose of this PR is to address these treat-warning-as-error problems. I'd like to rewrite this function in another PR.
>> 
>> I think we should add `#pragmas` one only as a last resort. We need to understand why the problem is happening.
>> 
>> This code has been there for a long time. I wonder what happened to cause it to fail in your build system. Could it be related to a particular version of VC++? I checked the build logs from Oracle's CI as well as GitHub actions
>> 
>> 19.27.29111 Oracle  -- OK
>> 19.28.29334 Alibaba -- warnings
>> 19.28.29910 GitHub  -- OK
>> 
>> Or, is it related to the system language setting of your build machine (e.g., could it be set to Chinese?)
>> 
>> Most importantly, does the `#pragma` actually make the code work, or does it merely hides the problem? I.e., will sscanf fail at runtime when it sees the `%&`? Have you run any tests to validate that the affected code works with your toolchain?
>
> I have confirmed that this problem is related to a specific msvc version(At least it happens in msvc 19.28.29334, I have not tested other msvc versions). After applying this patch, I can build successfully, both on upstream JDK and Alibaba JDK.
> 
> I have written a minimal reproducible demo. When the /WX(Treats all compiler warnings as errors) option is turned on, the compiler issued the same warnings. After disabling these warnings via `#pragma`, the compiler will not complain about anything, the execution result is exactly as I expected. Besides, all test cases under `compiler/compilercontrol/` are passed except ClearDirectivesFileStackTest.java which has been problem-listed before. So I believe this is a false positive of the compilation warning, turning it off or on will not affect the runtime behavior.
> 
> FYI: See detailed jtreg log and minimal reproducible demo on JBS attachments.
> 
> Thanks,
> Yang

I happened to have the exact same version of cl.exe (19.28.29334) but have not been seeing this issue, and I can't reproduce it with the reproducer you attached to the JBS issue either.

I do note that the reproducer has a bunch of `LS` characters where `LF` are expected, and Visual Studio asks if I want to normalize them, but, whether I do that or not doesn't change the outcome. I just don't get the same warnings.

So, it seems like there might be an environment/configuration problem somewhere, and these warnings are just a symptom of that. It might be worth it to investigate further.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3107