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

[Yi Yang]

On Tue, 23 Mar 2021 03:06:42 GMT, Ioi Lam <iklam 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.
>
>> > 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

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

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