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

Closed
wants to merge 5 commits into from

Conversation

rnkovacs
Copy link
Contributor

@rnkovacs rnkovacs commented Aug 23, 2021

Despite its title, this patch only touches common code that's present before adding Windows/AArch64 support. Not a clean backport. Differences to the original commit:

  • This is the first time we disable an MSVC warning for hotspot in 11, so in make/hotspot/lib/CompileJvm.gmk this introduces a new DISABLED_WARNINGS_microsoft line.
  • We disable the same warning in make/hotspot/lib/CompileGtest.gmk.
  • A tiny useless switch is removed from src/hotspot/cpu/aarch64/aarch64.ad that's no longer present on tip (probably not worth a separate PR).
  • Changes to src/hotspot/cpu/aarch64/frame_aarch64.cpp don't yet include the changes from 8248414: AArch64: Remove uses of long and unsigned long ints #215.
  • The assert on line 1477 in src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp needed some adjustment to compile. (I'm not sure how it compiles on tip, as I don't see -Wsign-compare turned off there either, but the types seem to be the same.)

This is part of the Windows/AArch64 port.


Progress

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

Issue

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

Reviewers

Contributors

  • Bernhard Urban-Forster <burban@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk11u-dev pull/280/head:pull/280
$ git checkout pull/280

Update a local copy of the PR:
$ git checkout pull/280
$ git pull https://git.openjdk.java.net/jdk11u-dev pull/280/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 280

View PR using the GUI difftool:
$ git pr show -t 280

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk11u-dev/pull/280.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 23, 2021

👋 Welcome back rnkovacs! 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 changed the title Backport d2812f780e87511736f2d41bc57a69c2a898b583 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build Aug 23, 2021
@openjdk
Copy link

openjdk bot commented Aug 23, 2021

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added the backport label Aug 23, 2021
@rnkovacs
Copy link
Contributor Author

/contributor add Bernhard Urban-Forster burban@openjdk.org

@rnkovacs rnkovacs marked this pull request as ready for review Aug 23, 2021
@openjdk
Copy link

openjdk bot commented Aug 23, 2021

@rnkovacs
Contributor Bernhard Urban-Forster <burban@openjdk.org> successfully added.

@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 23, 2021
@mlbridge
Copy link

mlbridge bot commented Aug 23, 2021

Webrevs

@RealCLanger
Copy link
Contributor

@rnkovacs can you update this PR as well to resolve the merge conflicts?

@rnkovacs
Copy link
Contributor Author

I did, thanks.

lewurm
lewurm approved these changes Oct 1, 2021
Copy link
Member

@lewurm lewurm left a comment

Choose a reason for hiding this comment

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

Looks good to me (but I'm not a formal reviewer).

make/hotspot/lib/CompileGtest.gmk Show resolved Hide resolved
make/hotspot/lib/CompileJvm.gmk Show resolved Hide resolved
@@ -1333,7 +1333,7 @@ class StubGenerator: public StubCodeGenerator {
// disjoint_int_copy_entry is set to the no-overlap entry point
// used by generate_conjoint_int_oop_copy().
//
address generate_disjoint_copy(size_t size, bool aligned, bool is_oop, address *entry,
address generate_disjoint_copy(int size, bool aligned, bool is_oop, address *entry,
Copy link
Contributor

Choose a reason for hiding this comment

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

size_t is correct here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched back: 09587d5
This way, however, I had to add casts to the two call sites where size is used, to avoid a size_t to int truncation warning (C4267). Both calls expect integers. Making size an int here avoided both the warning and the casts.

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 2, 2021

@rnkovacs This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 30, 2021

@rnkovacs This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Nov 30, 2021
@rnkovacs
Copy link
Contributor Author

/open

@openjdk openjdk bot reopened this Feb 11, 2022
@openjdk
Copy link

openjdk bot commented Feb 11, 2022

@rnkovacs This pull request is now open

@rnkovacs
Copy link
Contributor Author

Sorry for the delay. The new changes are:

assert(JavaThread::stack_shadow_zone_size() == (unsigned long)(int)JavaThread::stack_shadow_zone_size(), "must be same");
__ bang_stack_with_offset((int)JavaThread::stack_shadow_zone_size());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert(JavaThread::stack_shadow_zone_size() == (unsigned long)(int)JavaThread::stack_shadow_zone_size(), "must be same");
__ bang_stack_with_offset((int)JavaThread::stack_shadow_zone_size());
__ bang_stack_with_offset(checked_cast<int>(JavaThread::stack_shadow_zone_size()));

@@ -1363,12 +1363,12 @@ class StubGenerator: public StubCodeGenerator {
// save regs before copy_memory
__ push(RegSet::of(d, count), sp);
}
copy_memory(aligned, s, d, count, rscratch1, size);
copy_memory(aligned, s, d, count, rscratch1, (int)size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
copy_memory(aligned, s, d, count, rscratch1, (int)size);
copy_memory(aligned, s, d, count, rscratch1, checked_cast<int>(size));


if (is_oop) {
__ pop(RegSet::of(d, count), sp);
if (VerifyOops)
verify_oop_array(size, d, count, r16);
verify_oop_array((int)size, d, count, r16);
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise use checked_cast.

Copy link
Contributor

@theRealAph theRealAph left a comment

Choose a reason for hiding this comment

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

If you replace all of the int casts with checked_cast<int>, I'll approve this.

@rnkovacs
Copy link
Contributor Author

I replaced them. Thanks for the review.

@openjdk
Copy link

openjdk bot commented Feb 13, 2022

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

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

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

Co-authored-by: Bernhard Urban-Forster <burban@openjdk.org>
Reviewed-by: burban, 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 2 new commits pushed to the master branch:

  • 0aa5f75: 8262894: [macos_aarch64] SIGBUS in Assembler::ld_st2
  • 69583df: 8266889: [macosx-aarch64] Crash with SIGBUS in MarkActivationClosure::do_code_blob during vmTestbase/nsk/jvmti/.../bi04t002 test run

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 (@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 sponsor Pull request is ready to be sponsored label Feb 14, 2022
@openjdk
Copy link

openjdk bot commented Feb 14, 2022

@rnkovacs
Your change (at version c5f9428) is now ready to be sponsored by a Committer.

@rnkovacs
Copy link
Contributor Author

Rebased to latest master to fix merge conflict.

@openjdk openjdk bot removed the sponsor Pull request is ready to be sponsored label Feb 15, 2022
@rnkovacs
Copy link
Contributor Author

The one test that fails on Linux x64 and x86 (runtime/Thread/StopAtExit.java) is not specific to this patch, it also fails for this unrelated PR: #817

Could someone please sponsor the change? @theRealAph or @GoeLin maybe?

@VladimirKempik
Copy link

it lost label "sponsor", so you probably need to /integrate again

@adinn
Copy link
Contributor

adinn commented Feb 16, 2022

/sponsor

@openjdk
Copy link

openjdk bot commented Feb 16, 2022

@adinn The PR has been updated since the change author (@rnkovacs) issued the integrate command - the author must perform this command again.

@rnkovacs
Copy link
Contributor Author

Ah right, thanks @VladimirKempik.

/integrate

@openjdk openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 16, 2022
@openjdk
Copy link

openjdk bot commented Feb 16, 2022

@rnkovacs
Your change (at version ec4df85) is now ready to be sponsored by a Committer.

@rnkovacs
Copy link
Contributor Author

Sorry @adinn - could you please try again?

@VladimirKempik
Copy link

/sponsor

@openjdk
Copy link

openjdk bot commented Feb 16, 2022

Going to push as commit b5f8c31.
Since your change was applied there have been 2 commits pushed to the master branch:

  • 0aa5f75: 8262894: [macos_aarch64] SIGBUS in Assembler::ld_st2
  • 69583df: 8266889: [macosx-aarch64] Crash with SIGBUS in MarkActivationClosure::do_code_blob during vmTestbase/nsk/jvmti/.../bi04t002 test run

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Feb 16, 2022
@openjdk openjdk bot closed this Feb 16, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Feb 16, 2022
@openjdk
Copy link

openjdk bot commented Feb 16, 2022

@VladimirKempik @rnkovacs Pushed as commit b5f8c31.

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

@rnkovacs
Copy link
Contributor Author

Thanks so much @VladimirKempik!

@@ -89,7 +89,7 @@ class MacroAssembler: public Assembler {
= (operand_valid_for_logical_immediate(false /*is32*/,
(uint64_t)Universe::narrow_klass_base())
&& ((uint64_t)Universe::narrow_klass_base()
> (1UL << log2_intptr((uintptr_t)Universe::narrow_klass_range()))));
Copy link

Choose a reason for hiding this comment

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

I am getting a build issue when building on MacOS/aarch64 12.2.1 using Xcode 13.1. The cause being the uintptr_t cast that was removed from line 92.

I resolved the issue by adding the cast back like so:
> (1UL << log2_intptr(checked_cast<uintptr_t>(Universe::narrow_klass_range())))));

If all looks well, I will have a colleague with Author status go ahead and create an issue on JBS for me so I can then submit the fix.

Here's the build error:

 $ make images CONF=macosx-aarch64-normal-server-release
Building target 'images' in configuration 'macosx-aarch64-normal-server-release'
In file included from /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/precompiled/precompiled.hpp:111:
In file included from /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/interpreter/abstractInterpreter.hpp:28:
In file included from /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/asm/macroAssembler.hpp:31:
/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp:92:25: error: call to 'log2_intptr' is ambiguous
             > (1ULL << log2_intptr(Universe::narrow_klass_range()))));
                        ^~~~~~~~~~~
In file included from /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/precompiled/precompiled.hpp:111:
In file included from /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/interpreter/abstractInterpreter.hpp:28:
In file included from /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/asm/macroAssembler.hpp:31:
/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp:92:25: error: call to 'log2_intptr' is ambiguous
             > (1ULL << log2_intptr(Universe::narrow_klass_range()))));
                        ^~~~~~~~~~~
/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/utilities/globalDefinitions.hpp:1094:12: note: candidate function
inline int log2_intptr(uintptr_t x) {
           ^
/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/utilities/globalDefinitions.hpp:1120:12: note: candidate function
inline int log2_intptr(intptr_t x) {
           ^
/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/utilities/globalDefinitions.hpp:1094:12: note: candidate function
inline int log2_intptr(uintptr_t x) {
           ^
/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/utilities/globalDefinitions.hpp:1120:12: note: candidate function
inline int log2_intptr(intptr_t x) {
           ^
1 error generated.
1 error generated.
make[3]: *** [/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/build/macosx-aarch64-normal-server-release/hotspot/variant-server/libjvm/gtest/objs/precompiled/precompiled.hpp.pch] Error 1
make[3]: *** Waiting for unfinished jobs....
make[3]: *** [/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/build/macosx-aarch64-normal-server-release/hotspot/variant-server/libjvm/objs/precompiled/precompiled.hpp.pch] Error 1
make[2]: *** [hotspot-server-libs] Error 2

ERROR: Build failed for target 'images' in configuration 'macosx-aarch64-normal-server-release' (exit code 2)

=== Output from failing command(s) repeated here ===
* For target hotspot_variant-server_libjvm_gtest_objs_precompiled_precompiled.hpp.pch:
In file included from /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/precompiled/precompiled.hpp:111:
In file included from /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/interpreter/abstractInterpreter.hpp:28:
In file included from /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/asm/macroAssembler.hpp:31:
/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp:92:25: error: call to 'log2_intptr' is ambiguous
             > (1ULL << log2_intptr(Universe::narrow_klass_range()))));
                        ^~~~~~~~~~~
/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/utilities/globalDefinitions.hpp:1094:12: note: candidate function
inline int log2_intptr(uintptr_t x) {
           ^
/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/utilities/globalDefinitions.hpp:1120:12: note: candidate function
inline int log2_intptr(intptr_t x) {
           ^
1 error generated.
* For target hotspot_variant-server_libjvm_objs_precompiled_precompiled.hpp.pch:
In file included from /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/precompiled/precompiled.hpp:111:
In file included from /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/interpreter/abstractInterpreter.hpp:28:
In file included from /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/asm/macroAssembler.hpp:31:
/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp:92:25: error: call to 'log2_intptr' is ambiguous
             > (1ULL << log2_intptr(Universe::narrow_klass_range()))));
                        ^~~~~~~~~~~
/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/utilities/globalDefinitions.hpp:1094:12: note: candidate function
inline int log2_intptr(uintptr_t x) {
           ^
/Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/src/hotspot/share/utilities/globalDefinitions.hpp:1120:12: note: candidate function
inline int log2_intptr(intptr_t x) {
           ^
1 error generated.

* All command lines available in /Users/ahmed/Documents/dev/repos/jdk11u-dev-upstream/build/macosx-aarch64-normal-server-release/make-support/failure-logs.
=== End of repeated output ===

No indication of failed target found.
Hint: Try searching the build log for '] Error'.
Hint: See doc/building.html#troubleshooting for assistance.

make[1]: *** [main] Error 2
make: *** [images] Error 2

@theRealAph
Copy link
Contributor

theRealAph commented Feb 24, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport integrated Pull request has been integrated
7 participants