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

2020-11-03 Thread Andrew Haley 
On 02/11/2020 18:37, Bernhard Urban-Forster wrote:
> @theRealAph what gcc version?
> 
> I can reproduce with
> $ gcc --version
> gcc (Ubuntu 9.2.1-9ubuntu2) 9.2.1 20191008
> which ships in Ubuntu 19.10 as default

My mistake: I think it only triggers with a debug build, because assert
is a macro.

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

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

2020-11-02 Thread Bernhard Urban-Forster 
On Mon, 2 Nov 2020 17:43:31 GMT, Andrew Haley  wrote:

>> https://github.com/openjdk/jdk/pull/1013
>
>> @lewurm
>> This patch seems to break on linux-aarch64 with gcc:
> 
> Builds cleanly on Linux/GCC or me.

@theRealAph what gcc version?

I can reproduce with
$ gcc --version
gcc (Ubuntu 9.2.1-9ubuntu2) 9.2.1 20191008
which ships in Ubuntu 19.10 as default

-

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

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

2020-11-02 Thread Andrew Haley 
On Mon, 2 Nov 2020 17:05:19 GMT, Bernhard Urban-Forster  
wrote:

>> @lewurm Open a new JBS issue with the bug. If you can find a fix in a short 
>> amount of time (which I would believe should be possible; probably just need 
>> a proper cast) it's acceptable to fix it directly. What amounts to a "short 
>> amount of time" is left to reasonable judgement; minutes to hours are okay, 
>> days are not.
>> 
>> Otherwise, create an anti-delta (revert changeset) to back out your changes, 
>> and open yet another JBS issue for re-implementing them correctly.  
>> 
>>  In this case, an alternative short-term fix could also be to remove the 
>> assert instead of backing out the entire patch.
>
> https://github.com/openjdk/jdk/pull/1013

> @lewurm
> This patch seems to break on linux-aarch64 with gcc:

Builds cleanly on Linux/GCC or me.

-

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

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

2020-11-02 Thread Bernhard Urban-Forster 
On Mon, 2 Nov 2020 16:16:25 GMT, Magnus Ihse Bursie  wrote:

>> @magicus I did test the initial version of this PR on linux+arm64, but not 
>> the latest iteration. sorry about that  
>> 
>> What is the policy here? Submit a revert right away or investigate a fix?
>
> @lewurm Open a new JBS issue with the bug. If you can find a fix in a short 
> amount of time (which I would believe should be possible; probably just need 
> a proper cast) it's acceptable to fix it directly. What amounts to a "short 
> amount of time" is left to reasonable judgement; minutes to hours are okay, 
> days are not.
> 
> Otherwise, create an anti-delta (revert changeset) to back out your changes, 
> and open yet another JBS issue for re-implementing them correctly.  
> 
>  In this case, an alternative short-term fix could also be to remove the 
> assert instead of backing out the entire patch.

https://github.com/openjdk/jdk/pull/1013

-

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

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

2020-11-02 Thread Magnus Ihse Bursie 
On Mon, 2 Nov 2020 16:06:15 GMT, Bernhard Urban-Forster  
wrote:

>> @lewurm 
>> This patch seems to break on linux-aarch64 with gcc:
>> open/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp:1501:52: error: 
>> comparison of integer expressions of different signedness: 'size_t' {aka 
>> 'long unsigned int'} and 'int' [-Werror=sign-compare]
>>  1501 | assert(StackOverflow::stack_shadow_zone_size() == 
>> (int)StackOverflow::stack_shadow_zone_size(), "must be same");
>>   |
>> ^~~
>> 
>> Did you test building this on any aarch64 platforms besides Windows?
>
> @magicus I did test the initial version of this PR on linux+arm64, but not 
> the latest iteration. sorry about that  
> 
> What is the policy here? Submit a revert right away or investigate a fix?

@lewurm Open a new JBS issue with the bug. If you can find a fix in a short 
amount of time (which I would believe should be possible; probably just need a 
proper cast) it's acceptable to fix it directly. What amounts to a "short 
amount of time" is left to reasonable judgement; minutes to hours are okay, 
days are not.

Otherwise, create an anti-delta (revert changeset) to back out your changes, 
and open yet another JBS issue for re-implementing them correctly.  

 In this case, an alternative short-term fix could also be to remove the assert 
instead of backing out the entire patch.

-

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

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

2020-11-02 Thread Bernhard Urban-Forster 
On Mon, 2 Nov 2020 15:41:06 GMT, Magnus Ihse Bursie  wrote:

>> Thank you Andrew.
>
> @lewurm 
> This patch seems to break on linux-aarch64 with gcc:
> open/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp:1501:52: error: 
> comparison of integer expressions of different signedness: 'size_t' {aka 
> 'long unsigned int'} and 'int' [-Werror=sign-compare]
>  1501 | assert(StackOverflow::stack_shadow_zone_size() == 
> (int)StackOverflow::stack_shadow_zone_size(), "must be same");
>   |
> ^~~
> 
> Did you test building this on any aarch64 platforms besides Windows?

@magicus I did test the initial version of this PR on linux+arm64, but not the 
latest iteration. sorry about that  

What is the policy here? Submit a revert right away or investigate a fix?

-

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

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

2020-11-02 Thread Magnus Ihse Bursie 
On Mon, 2 Nov 2020 14:00:33 GMT, Bernhard Urban-Forster  
wrote:

>>> Would you mind to sponsor it @theRealAph or @magicus?
>> 
>> Hmm, I think you have to integrate it first.
>> https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/sponsor
>
> Thank you Andrew.

@lewurm 
This patch seems to break on linux-aarch64 with gcc:
open/src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp:1501:52: error: 
comparison of integer expressions of different signedness: 'size_t' {aka 'long 
unsigned int'} and 'int' [-Werror=sign-compare]
 1501 | assert(StackOverflow::stack_shadow_zone_size() == 
(int)StackOverflow::stack_shadow_zone_size(), "must be same");
  |
^~~

Did you test building this on any aarch64 platforms besides Windows?

-

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

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

2020-11-02 Thread Bernhard Urban-Forster 
On Mon, 2 Nov 2020 13:41:53 GMT, Andrew Haley  wrote:

>> Marked as reviewed by aph (Reviewer).
>
>> Would you mind to sponsor it @theRealAph or @magicus?
> 
> Hmm, I think you have to integrate it first.
> https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/sponsor

Thank you Andrew.

-

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

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

2020-11-02 Thread Andrew Haley 
On Tue, 27 Oct 2020 14:04:04 GMT, Andrew Haley  wrote:

>> Bernhard Urban-Forster has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - uppercase suffix
>>  - add assert
>
> Marked as reviewed by aph (Reviewer).

> Would you mind to sponsor it @theRealAph or @magicus?

Hmm, I think you have to integrate it first.
https://wiki.openjdk.java.net/display/SKARA/Pull+Request+Commands#PullRequestCommands-/sponsor

-

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

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

2020-10-29 Thread Bernhard Urban-Forster 
On Tue, 27 Oct 2020 14:04:04 GMT, Andrew Haley  wrote:

>> Bernhard Urban-Forster has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - uppercase suffix
>>  - add assert
>
> Marked as reviewed by aph (Reviewer).

Would you mind sponsor it @theRealAph or @magicus?

-

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

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

2020-10-27 Thread Bernhard Urban-Forster 
On Tue, 27 Oct 2020 14:04:04 GMT, Andrew Haley  wrote:

>> Bernhard Urban-Forster has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - uppercase suffix
>>  - add assert
>
> Marked as reviewed by aph (Reviewer).

Thank you for the reviews, Magnus and Andrew!

-

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

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

2020-10-27 Thread Andrew Haley 
On Thu, 15 Oct 2020 18:35:30 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 incrementally with two 
> additional commits since the last revision:
> 
>  - uppercase suffix
>  - add assert

Marked as reviewed by aph (Reviewer).

-

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

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

2020-10-27 Thread Bernhard Urban-Forster 
On Sun, 18 Oct 2020 09:07:17 GMT, Magnus Ihse Bursie  wrote:

>> Bernhard Urban-Forster has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - uppercase suffix
>>  - add assert
>
> Build changes look fine now.

@theRealAph does the PR look okay to you now?

-

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

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

2020-10-18 Thread Magnus Ihse Bursie 
On Thu, 15 Oct 2020 18:35:30 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 incrementally with two 
> additional commits since the last revision:
> 
>  - uppercase suffix
>  - add assert

Build changes look fine now.

-

Marked as reviewed by ihse (Reviewer).

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

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

2020-10-15 Thread Bernhard Urban-Forster 
On Thu, 15 Oct 2020 17:24:56 GMT, Stuart Monteith  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 20 commits:
>>  - disable warning only for hotspot
>>  - Merge remote-tracking branch 'upstream/master' into 
>> 8254072-fix-windows-arm64-warnings
>>  - 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
>>  - ... and 10 more: 
>> https://git.openjdk.java.net/jdk/compare/9359ff03...32e922da
>
> src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp line 658:
> 
>> 656: }
>> 657:   }
>> 658:   size_t size_in_bytes() { return 1ull << size(); }
> 
> Capital ULL - I find that easer to search for and it is more consistent.

Thank you! Fixed.

-

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]

2020-10-15 Thread Bernhard Urban-Forster 
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 [v4]

2020-10-15 Thread Bernhard Urban-Forster 
> 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 incrementally with two 
additional commits since the last revision:

 - uppercase suffix
 - add assert

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/530/files
  - new: https://git.openjdk.java.net/jdk/pull/530/files/32e922da..901bbd48

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=530=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=530=02-03

  Stats: 2 lines in 2 files changed: 1 ins; 0 del; 1 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

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

2020-10-15 Thread Stuart Monteith 
On Thu, 15 Oct 2020 08:57:27 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 20 commits:
>  - disable warning only for hotspot
>  - Merge remote-tracking branch 'upstream/master' into 
> 8254072-fix-windows-arm64-warnings
>  - 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
>  - ... and 10 more: 
> https://git.openjdk.java.net/jdk/compare/9359ff03...32e922da

src/hotspot/cpu/aarch64/nativeInst_aarch64.hpp line 658:

> 656: }
> 657:   }
> 658:   size_t size_in_bytes() { return 1ull << size(); }

Capital ULL - I find that easer to search for and it is more consistent.

-

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]

2020-10-15 Thread Andrew Haley 
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]

2020-10-15 Thread Bernhard Urban-Forster 
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]

2020-10-15 Thread Bernhard Urban-Forster 
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 [v3]

2020-10-15 Thread Bernhard Urban-Forster 
> 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 20 commits:

 - disable warning only for hotspot
 - Merge remote-tracking branch 'upstream/master' into 
8254072-fix-windows-arm64-warnings
 - 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
 - ... and 10 more: https://git.openjdk.java.net/jdk/compare/9359ff03...32e922da

-

Changes: https://git.openjdk.java.net/jdk/pull/530/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=530=02
  Stats: 22 lines in 8 files changed: 1 ins; 0 del; 21 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

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

2020-10-12 Thread Magnus Ihse Bursie 
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

2020-10-09 Thread Andrew Haley 
On 08/10/2020 21:28, Bernhard Urban-Forster wrote:
> On Tue, 6 Oct 2020 18:09:05 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).
>
> Thank you Andrew for your comments!
>
>> _Mailing list message from [Andrew Haley](mailto:a...@redhat.com) on 
>> [hotspot-dev](mailto:hotspot-...@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://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

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

2020-10-08 Thread Bernhard Urban-Forster 
> 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

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

2020-10-08 Thread Bernhard Urban-Forster 
On Tue, 6 Oct 2020 18:09:05 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).

Thank you Andrew for your comments!

> _Mailing list message from [Andrew Haley](mailto:a...@redhat.com) on 
> [hotspot-dev](mailto:hotspot-...@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?



For the remaining comments, I've updated the PR, please have another look.

-

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