RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build

[Andrew Haley]

On 08/10/2020 21:28, Bernhard Urban-Forster wrote:
> On Tue, 6 Oct 2020 18:09:05 GMT, Bernhard Urban-Forster <burban at openjdk.org> wrote:
>
>> I organized this PR so that each commit contains the warning emitted by MSVC as commit message and its relevant fix.
>>
>> Verified on
>> * Linux+ARM64: `{hotspot,jdk,langtools}:tier1`, no failures.
>> * Windows+ARM64: `{hotspot,jdk,langtools}:tier1`, no (new) failures.
>> * internal macOS+ARM64 port: build without `--disable-warnings-as-errors` still works. Just mentioning this here, because
>>   it's yet another toolchain (Xcode / clang) that needs to be kept happy [going
>>   forward](https://openjdk.java.net/jeps/391).
>
> Thank you Andrew for your comments!
>
>> _Mailing list message from [Andrew Haley](mailto:aph at redhat.com) on [hotspot-dev](mailto:hotspot-dev at openjdk.java.net):_
>> IMO this warning:
>>
>> warning C4146: unary minus operator applied to unsigned type, result still unsigned
>>
>> should not be used.
>
> Okay, added to the Makefile and reverted those changes.
>
>> // Generate stack overflow check
>> if (UseStackBanging) {
>> - __ bang_stack_with_offset(JavaThread::stack_shadow_zone_size());
>> + __ bang_stack_with_offset((int)JavaThread::stack_shadow_zone_size());
>> } else {
>> Unimplemented();
>>
>> Could this one be fixed by changing stack_shadow_zone_size() or
>> bang_stack_with_offset() ? I would have thought that whatever type
>> stack_shadow_zone_size() returns should be compatible with
>> bang_stack_with_offset().
>
> The x86_64 backend and others do the same:
> https://github.com/openjdk/jdk/blob/5351ba6cfa8078f503f1cf0c375b692905c607ff/src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp#L2176-L2178
>
> So should we (1) do the same, (2) diverge or (3) fix all of them?

I hate changing code just to silence compiler warnings. Occasionally,
these warnings find real bugs, but there have been several important
programs broken by silencing compiler warnings.
(http://taint.org/2008/05/13/153959a.html is the most famous.)

The problem with "fixing all of them" is that it's real work, because
inevitably some of the instructions in the various back ends will take
int arguments, so there will be several things to fix.

Whenever making changes to "shut the compiler up", as is the case
here, we have to consider what the real problem is rather than just
throwing in casts.

In this case, we "know" that stack_shadow_zone_size() will fit into an
int, so there is not a problem. But stack_shadow_zone_size() returns a
size_t, and all of the logic used to calculate it is very careful to
maintain this. There's a check in bang_stack_with_offset() to make
sure offset is positive, which is rather pointless. Maybe the right
thing to do is change our bang_stack_with_offset() to take a size_t
and fix (or remove) the sanity check. Bear in mind that if you keep a
sanity check, pages can be up to a megabyte in size, so you have to
consider what the assertion is for.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671