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

[Andrew Haley]

On Thu, 15 Oct 2020 09:02:35 GMT, Bernhard Urban-Forster <burban at openjdk.org> wrote:

>> Changes requested by ihse (Reviewer).
>
> @theRealAph I prototyped changing the argument of `bang_stack_with_offset()` from `int` to `size_t` here:
> https://github.com/lewurm/openjdk/commit/85a8f655aa0cb69ef13a2de44dd64c60caf19852. In that approach casting is
> essentially pushed down to `bang_stack_with_offset` because the assembler instruction of most (all) architectures that
> is eventually consuming that offset needs a signed integer anyway. Doesn't seem like a win to me to be honest.  I would
> rather prefer to go with what we have in this patch (similar to what x86 is doing today):
> --- a/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
> +++ b/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
> @@ -1524,7 +1524,7 @@ nmethod* SharedRuntime::generate_native_wrapper(MacroAssembler* masm,
> 
>    // 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();
>    }
> and leave it with that. What do you think?

> @theRealAph I prototyped changing the argument of `bang_stack_with_offset()` from `int` to `size_t` here:
> [lewurm at 85a8f65](https://github.com/lewurm/openjdk/commit/85a8f655aa0cb69ef13a2de44dd64c60caf19852). In that approach
> casting is essentially pushed down to `bang_stack_with_offset` because the assembler instruction of most (all)
> architectures that is eventually consuming that offset needs a signed integer anyway. Doesn't seem like a win to me to
> be honest.  I would rather prefer to go with what we have in this patch (similar to what x86 is doing today):  ```diff
> --- a/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
> +++ b/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp
> @@ -1524,7 +1524,7 @@ nmethod* SharedRuntime::generate_native_wrapper(MacroAssembler* masm,
> 
>    // 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();
>    }
> ```
> 
> and leave it with that. What do you think?

Fine, but please assert `JavaThread::stack_shadow_zone_size() == (int)JavaThread::stack_shadow_zone_size()`.

If all this sounds a bit paranoid, that's because I am.

Adding casts to shut up compilers is a very risky business, because often (if not in this case) the programmer doesn't
understand the code well, and just sprinkles casts everywhere. But those casts disable compile-time type checking, and
this leads to risks for future maintainability.

I wonder if we should fix this in a better way, and use this in the future:

template <typename T1, typename T2>
T1 checked_cast(T2 thing) {
  guarantee(static_cast<T1>(thing) == thing, "must be");
  return static_cast<T1>(thing);
}
Then I promise we'll never need to have this conversation again.

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

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