Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

lewurm
Copy link
Member

@lewurm lewurm commented Oct 6, 2020

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.

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

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

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/530/head:pull/530
$ git checkout pull/530

lewurm added 14 commits October 6, 2020 16:48
… '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
…nary minus operator applied to unsigned type, result still unsigned
…'<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
…'|': check operator precedence for possible error; use parentheses to clarify precedence
…ntf' : format string '%016lx' requires an argument of type 'unsigned long', but variadic argument 1 has type 'uintptr_t'

./src/hotspot/cpu/aarch64/frame_aarch64.cpp(686): note: consider using '%llx' in the format string
./src/hotspot/cpu/aarch64/frame_aarch64.cpp(686): note: consider using '%Ix' in the format string
./src/hotspot/cpu/aarch64/frame_aarch64.cpp(686): note: consider using '%I64x' in the format string
…y minus operator applied to unsigned type, result still unsigned

m
…nary minus operator applied to unsigned type, result still unsigned
…4267: 'initializing': conversion from 'size_t' to 'unsigned int', possible loss of data

./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(1837): warning C4267: 'initializing': conversion from 'size_t' to 'const unsigned int', possible loss of data
…4146: unary minus operator applied to unsigned type, result still unsigned
…4267: '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
…267: 'argument': conversion from 'size_t' to 'int', possible loss of data
…312: 'type cast': conversion from 'unsigned int' to 'address' of greater size
…267: '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
…23): 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
@bridgekeeper
Copy link

bridgekeeper bot commented Oct 6, 2020

👋 Welcome back burban! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 6, 2020
@openjdk
Copy link

openjdk bot commented Oct 6, 2020

@lewurm The following label will be automatically applied to this pull request:

  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the hotspot hotspot-dev@openjdk.org label Oct 6, 2020
@lewurm lewurm changed the title 8254072: Get rid of --disable-warnings-as-errors on Windows+ARM64 build 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build Oct 6, 2020
@mlbridge
Copy link

mlbridge bot commented Oct 6, 2020

Webrevs

@mlbridge
Copy link

mlbridge bot commented Oct 7, 2020

Mailing list message from Andrew Haley on hotspot-dev:

On 06/10/2020 19:17, 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).

Some of these don't look right.

@@ -69,7 +69,7 @@ int compare_immediate_pair(const void *i1, const void *i2)
// for i = 1, ... N result<i-1> = 1 other bits are zero
static inline uint64_t ones(int N)
{
- return (N == 64 ? -1ULL : (1ULL << N) - 1);
+ return (N == 64 ? ~0 : (1ULL << N) - 1);
}

Turns out this does work because ~0 is a signed quantity which is then
sign extended to 64 bits, then converted to unsigned. But his is
obscure and therefore risky coding style, worse that what it replaces.

IMO this warning:

warning C4146: unary minus operator applied to unsigned type, result still unsigned

should not be used. There is nothing wrong with negating an unsigned
value: doing so is well defined in all cases. Do the authors of MSVC
not understand the language? Or do they think their users do not
understand the language?

Please have a look to see how many of these diffs would go away with
that particular warning disabled.

@@ -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();

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().

@@ -1309,7 +1309,7 @@ class StubGenerator: public StubCodeGenerator {
__ ldrw(r16, Address(a, rscratch2, Address::lsl(exact_log2(size))));
__ decode_heap_oop(temp); // calls verify_oop
}
- __ add(rscratch2, rscratch2, size);
+ __ add(rscratch2, rscratch2, (int)size);
__ b(loop);
__ bind(end);

Definitely not.

@@ -1367,7 +1367,7 @@ class StubGenerator: public StubCodeGenerator {
// UnsafeCopyMemory page error: continue after ucm
bool add_entry = !is_oop && (!aligned || sizeof(jlong) == size);
UnsafeCopyMemoryMark ucmm(this, add_entry, true);
- copy_memory(aligned, s, d, count, rscratch1, size);
+ copy_memory(aligned, s, d, count, rscratch1, (int)size);
}

Better to fix the type of size. The problem seems to be that it's
passed as a size_t to generate_conjoint_copy() but it's used as an int
elsewhere.

--
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

lewurm added 4 commits October 8, 2020 20:56
…267: '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
@openjdk openjdk bot added the build build-dev@openjdk.org label Oct 8, 2020
@lewurm
Copy link
Member Author

lewurm commented Oct 8, 2020

Thank you Andrew for your comments!

Mailing list message from Andrew Haley on hotspot-dev:
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:

if (UseStackBanging) {
__ bang_stack_with_offset((int)StackOverflow::stack_shadow_zone_size());
} else {

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.

@mlbridge
Copy link

mlbridge bot commented Oct 9, 2020

Mailing list message from Andrew Haley on hotspot-dev:

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

@theRealAph
Copy link
Contributor

theRealAph commented Oct 15, 2020

@theRealAph I prototyped changing the argument of bang_stack_with_offset() from int to size_t here: lewurm@85a8f65. 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?

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 sprinkles casts everywhere. But casts disable compile-time type checking, and this leads to risks for future maintainability.

I wonder if we should fix it in a better way, and use something like
this in the future for all compiler warnings:

template <typename T1, typename T2>
T1 checked_cast(T2 thing) {
  T1 result = static_cast<T1>(thing);
  guarantee(static_cast<T2>(result) == thing, "must be");
  return result;
}

I know this is additional work, but I promise we'll never need to have this conversation again.

@lewurm
Copy link
Member Author

lewurm commented Oct 15, 2020

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 <typename T1, typename T2>
T1 checked_cast(T2 thing) {
  T1 result = static_cast<T1>(thing);
  guarantee(static_cast<T2>(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 to track it.

Copy link
Member

@magicus magicus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build changes look fine now.

@openjdk
Copy link

openjdk bot commented Oct 18, 2020

@lewurm This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

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

Reviewed-by: ihse, aph

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 206 new commits pushed to the master branch:

  • a023b93: 8255394: jdk/test/lib/hexdump/ASN1FormatterTest.java fails with ---illegal-access=deny
  • 84e985d: 8253920: Share method trampolines in CDS dynamic archive
  • 7d41a54: 8255450: runtime/ThreadCountLimit.java causes high system load
  • 504cb00: 8252113: Move jfr man page into jfr module
  • 552192f: 8255305: Add Linux x86_32 tier1 to submit workflow
  • 66a3917: 8255331: Problemlist java/foreign/TestMismatch.java on 32-bit platforms until JDK-8254162
  • cf56c7e: 8254980: ZGC: ZHeapIterator visits armed nmethods with -XX:-ClassUnloading
  • 18d9905: 8255342: Remove non-specified JVM checks on Classes with Record attributes
  • 7679650: 8231231: The printing result is different from the case instruction
  • f7c59c6: 8255231: Avoid upcalls when initializing the statSampler
  • ... and 196 more: https://git.openjdk.java.net/jdk/compare/9359ff03ae6b9e09e7defef148864f40e949b669...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@magicus, @theRealAph) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Oct 18, 2020
@lewurm
Copy link
Member Author

lewurm commented Oct 27, 2020

@theRealAph does the PR look okay to you now?

@lewurm
Copy link
Member Author

lewurm commented Oct 27, 2020

/integrate

Thank you for the reviews, Magnus and Andrew!

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 27, 2020
@openjdk
Copy link

openjdk bot commented Oct 27, 2020

@lewurm
Your change (at version 901bbd4) is now ready to be sponsored by a Committer.

@lewurm
Copy link
Member Author

lewurm commented Oct 29, 2020

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

@theRealAph
Copy link
Contributor

/sponsor

@theRealAph
Copy link
Contributor

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

@openjdk openjdk bot closed this Nov 2, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 2, 2020
@openjdk
Copy link

openjdk bot commented Nov 2, 2020

@theRealAph @lewurm Since your change was applied there have been 291 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit d2812f7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk-notifier bot referenced this pull request Nov 2, 2020
@lewurm
Copy link
Member Author

lewurm commented Nov 2, 2020

Thank you Andrew.

@magicus
Copy link
Member

magicus commented Nov 2, 2020

@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?

@lewurm
Copy link
Member Author

lewurm commented Nov 2, 2020

@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?

@magicus
Copy link
Member

magicus commented Nov 2, 2020

@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.

@lewurm
Copy link
Member Author

lewurm commented Nov 2, 2020

#1013

@theRealAph
Copy link
Contributor

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

Builds cleanly on Linux/GCC or me.

@lewurm
Copy link
Member Author

lewurm commented Nov 2, 2020

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
Development

Successfully merging this pull request may close these issues.

4 participants