Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v2]
On Thu, 15 Oct 2020 09:57:14 GMT, Andrew Haley wrote:
> Fine, but please assert JavaThread::stack_shadow_zone_size() ==
> (int)JavaThread::stack_shadow_zone_size().
Done.
> 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 sprinkles casts everywhere. But casts disable
> compile-time type checking, and this leads
> to risks for future maintainability.
Full ACK and I appreciate your comments on this!
> I wonder if we should fix it in a better way, and use something like
> this in the future for all compiler warnings:
>
> ```
> template
> T1 checked_cast(T2 thing) {
> T1 result = static_cast(thing);
> guarantee(static_cast(result) == thing, "must be");
> return result;
> }
> ```
>
> I know this is additional work, but I promise we'll never need to have this
> conversation again.
This sounds like a great idea to me. I assume it doesn't fit into the scope of
this PR, therefore I've created
[JDK-8254856](https://bugs.openjdk.java.net/browse/JDK-8254856) to track it.
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v2]
On Thu, 15 Oct 2020 09:02:35 GMT, Bernhard Urban-Forster
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@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
T1 checked_cast(T2 thing) {
guarantee(static_cast(thing) == thing, "must be");
return static_cast(thing);
}
Then I promise we'll never need to have this conversation again.
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v2]
On Mon, 12 Oct 2020 10:29:23 GMT, Magnus Ihse Bursie wrote:
>> Bernhard Urban-Forster has updated the pull request with a new target base
>> due to a merge or a rebase. The pull request
>> now contains 18 commits:
>> - Merge remote-tracking branch 'upstream/master' into
>> 8254072-fix-windows-arm64-warnings
>> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'int', possible loss of data
>>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1446): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'int', possible loss of data
>> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1654): warning C4267:
>> 'argument':
>>conversion from 'size_t' to 'int', possible loss of data
>> - Revert changes for "warning C4146: unary minus operator applied to
>> unsigned type, result still unsigned"
>> - msvc: disable unary minus warning for unsigned types
>> - ./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
>> warning C4267: 'initializing': conversion
>>from 'size_t' to 'int', possible loss of data
>>./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
>> warning C4267: 'initializing': conversion
>>from 'size_t' to 'const int', possible loss of data
>> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1312): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'unsigned int', possible loss of data
>>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1370): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'int', possible loss of data
>> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4146:
>> unary minus
>>operator applied to unsigned type, result still unsigned
>> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441):
>>warning C4267: 'argument': conversion from 'size_t' to 'int', possible
>> loss of data
>> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(2472): warning C4312:
>> 'type cast': conversion from 'unsigned int'
>>to 'address' of greater size
>> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(1527): warning C4267:
>> 'argument': conversion from 'size_t' to
>>'int', possible loss of data
>> - ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning
>> C4267: 'initializing': conversion from 'size_t' to
>>'int', possible loss of data
>>./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning
>> C4267: 'initializing': conversion from 'size_t' to
>>'const int', possible loss of data
>> - ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2756): warning
>> C4146: unary minus operator applied to unsigned
>>type, result still unsigned
>> - ... and 8 more:
>> https://git.openjdk.java.net/jdk/compare/5351ba6c...a081dfb4
>
> 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?
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v2]
On Mon, 12 Oct 2020 10:29:11 GMT, Magnus Ihse Bursie wrote: >> Bernhard Urban-Forster has updated the pull request with a new target base >> due to a merge or a rebase. The pull request >> now contains 18 commits: >> - Merge remote-tracking branch 'upstream/master' into >> 8254072-fix-windows-arm64-warnings >> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4267: >> 'argument': conversion from 'size_t' to >>'int', possible loss of data >>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1446): warning C4267: >> 'argument': conversion from 'size_t' to >>'int', possible loss of data >> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1654): warning C4267: >> 'argument': >>conversion from 'size_t' to 'int', possible loss of data >> - Revert changes for "warning C4146: unary minus operator applied to >> unsigned type, result still unsigned" >> - msvc: disable unary minus warning for unsigned types >> - ./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123): >> warning C4267: 'initializing': conversion >>from 'size_t' to 'int', possible loss of data >>./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123): >> warning C4267: 'initializing': conversion >>from 'size_t' to 'const int', possible loss of data >> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1312): warning C4267: >> 'argument': conversion from 'size_t' to >>'unsigned int', possible loss of data >>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1370): warning C4267: >> 'argument': conversion from 'size_t' to >>'int', possible loss of data >> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4146: >> unary minus >>operator applied to unsigned type, result still unsigned >> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): >>warning C4267: 'argument': conversion from 'size_t' to 'int', possible >> loss of data >> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(2472): warning C4312: >> 'type cast': conversion from 'unsigned int' >>to 'address' of greater size >> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(1527): warning C4267: >> 'argument': conversion from 'size_t' to >>'int', possible loss of data >> - ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning >> C4267: 'initializing': conversion from 'size_t' to >>'int', possible loss of data >>./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning >> C4267: 'initializing': conversion from 'size_t' to >>'const int', possible loss of data >> - ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2756): warning >> C4146: unary minus operator applied to unsigned >>type, result still unsigned >> - ... and 8 more: >> https://git.openjdk.java.net/jdk/compare/5351ba6c...a081dfb4 > > make/autoconf/flags-cflags.m4 line 137: > >> 135: WARNINGS_ENABLE_ALL="-W3" >> 136: DISABLED_WARNINGS="4800" >> 137: DISABLED_WARNINGS+=" 4146" # unary minus operator applied to >> unsigned type, result still unsigned > > This change will affect *all* JDK code. I'm not sure this was intended? > > If it was intended, I think you need to motivate this more explicitly. > > If you only wanted to disable the warning for hotspot, the proper solution > would be to add it to > DISABLED_WARNINGS_microsoft in make/hotspot/lib/CompileJvm.gmk. Thank you @magicus! It was indeed meant only for the hotspot part. - PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v2]
On Thu, 8 Oct 2020 20:28:33 GMT, Bernhard Urban-Forster
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).
>
> Bernhard Urban-Forster has updated the pull request with a new target base
> due to a merge or a rebase. The pull request
> now contains 18 commits:
> - Merge remote-tracking branch 'upstream/master' into
> 8254072-fix-windows-arm64-warnings
> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4267:
> 'argument': conversion from 'size_t' to
>'int', possible loss of data
>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1446): warning C4267:
> 'argument': conversion from 'size_t' to
>'int', possible loss of data
> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1654): warning C4267:
> 'argument':
>conversion from 'size_t' to 'int', possible loss of data
> - Revert changes for "warning C4146: unary minus operator applied to
> unsigned type, result still unsigned"
> - msvc: disable unary minus warning for unsigned types
> - ./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
> warning C4267: 'initializing': conversion
>from 'size_t' to 'int', possible loss of data
>./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
> warning C4267: 'initializing': conversion
>from 'size_t' to 'const int', possible loss of data
> - ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1312): warning C4267:
> 'argument': conversion from 'size_t' to
>'unsigned int', possible loss of data
>./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1370): warning C4267:
> 'argument': conversion from 'size_t' to
>'int', possible loss of data
> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4146:
> unary minus
>operator applied to unsigned type, result still unsigned
> ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441):
>warning C4267: 'argument': conversion from 'size_t' to 'int', possible
> loss of data
> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(2472): warning C4312:
> 'type cast': conversion from 'unsigned int'
>to 'address' of greater size
> - ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(1527): warning C4267:
> 'argument': conversion from 'size_t' to
>'int', possible loss of data
> - ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning C4267:
> 'initializing': conversion from 'size_t' to
>'int', possible loss of data
>./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning C4267:
> 'initializing': conversion from 'size_t' to
>'const int', possible loss of data
> - ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2756): warning C4146:
> unary minus operator applied to unsigned
>type, result still unsigned
> - ... and 8 more:
> https://git.openjdk.java.net/jdk/compare/5351ba6c...a081dfb4
Changes requested by ihse (Reviewer).
make/autoconf/flags-cflags.m4 line 137:
> 135: WARNINGS_ENABLE_ALL="-W3"
> 136: DISABLED_WARNINGS="4800"
> 137: DISABLED_WARNINGS+=" 4146" # unary minus operator applied to
> unsigned type, result still unsigned
This change will affect *all* JDK code. I'm not sure this was intended?
If it was intended, I think you need to motivate this more explicitly.
If you only wanted to disable the warning for hotspot, the proper solution
would be to add it to
DISABLED_WARNINGS_microsoft in make/hotspot/lib/CompileJvm.gmk.
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v2]
> 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).
Bernhard Urban-Forster has updated the pull request with a new target base due
to a merge or a rebase. The pull request
now contains 18 commits:
- Merge remote-tracking branch 'upstream/master' into
8254072-fix-windows-arm64-warnings
- ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4267:
'argument': conversion from 'size_t' to
'int', possible loss of data
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1446): warning C4267:
'argument': conversion from 'size_t' to
'int', possible loss of data
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1654): warning C4267:
'argument':
conversion from 'size_t' to 'int', possible loss of data
- Revert changes for "warning C4146: unary minus operator applied to unsigned
type, result still unsigned"
- msvc: disable unary minus warning for unsigned types
- ./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
warning C4267: 'initializing': conversion
from 'size_t' to 'int', possible loss of data
./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
warning C4267: 'initializing': conversion
from 'size_t' to 'const int', possible loss of data
- ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1312): warning C4267:
'argument': conversion from 'size_t' to
'unsigned int', possible loss of data
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1370): warning C4267:
'argument': conversion from 'size_t' to
'int', possible loss of data
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4146: unary
minus
operator applied to unsigned type, result still unsigned
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441):
warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss
of data
- ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(2472): warning C4312:
'type cast': conversion from 'unsigned int'
to 'address' of greater size
- ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(1527): warning C4267:
'argument': conversion from 'size_t' to
'int', possible loss of data
- ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning C4267:
'initializing': conversion from 'size_t' to
'int', possible loss of data
./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning C4267:
'initializing': conversion from 'size_t' to
'const int', possible loss of data
- ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2756): warning C4146:
unary minus operator applied to unsigned
type, result still unsigned
- ... and 8 more: https://git.openjdk.java.net/jdk/compare/5351ba6c...a081dfb4
-
Changes: https://git.openjdk.java.net/jdk/pull/530/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=530=01
Stats: 22 lines in 8 files changed: 2 ins; 0 del; 20 mod
Patch: https://git.openjdk.java.net/jdk/pull/530.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/530/head:pull/530
PR: https://git.openjdk.java.net/jdk/pull/530
