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

8257794: Zero: assert(istate->_stack_limit == istate->_thread->last_Java_sp() + 1) failed: wrong on Linux/x86_32 #1637

Closed
wants to merge 4 commits into from

Conversation

@DamonFool
Copy link
Member

@DamonFool DamonFool commented Dec 5, 2020

Hi all,

Zero debug build on Linux/x86_32 is broken due to an incorrect assert [1].

'istate->_stack_limit' is set here [2] as 'stack->sp() - 1'.
'istate->_thread->last_Java_sp()' is set here [3], which is actually 'stack->sp()' according to [4].

So the correct assert should be:
assert(istate->_stack_limit == istate->_thread->last_Java_sp() - 1)

It would be better to fix it.

Testing:

  • Zero fastdebug build passed on Linux/x86_32

Thanks.
Best regards,
Jie

[1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp#L422
[2] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/zero/zeroInterpreter_zero.cpp#L819
[3] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/zero/zeroInterpreter_zero.cpp#L194
[4] https://github.com/openjdk/jdk/blob/master/src/hotspot/os_cpu/linux_zero/thread_linux_zero.hpp#L65


Progress

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

Issue

  • JDK-8257794: Zero: assert(istate->_stack_limit == istate->_thread->last_Java_sp() + 1) failed: wrong on Linux/x86_32

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 5, 2020

👋 Welcome back jiefu! 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.

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Dec 5, 2020

/issue add JDK-8257794
/test
/label add hotspot-runtime
/cc hotspot-runtime

Loading

@openjdk openjdk bot added the rfr label Dec 5, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 5, 2020

@DamonFool This issue is referenced in the PR title - it will now be updated.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 5, 2020

@DamonFool
The hotspot-runtime label was successfully added.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 5, 2020

@DamonFool The hotspot-runtime label was already applied.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 5, 2020

Webrevs

Loading

@shipilev
Copy link
Contributor

@shipilev shipilev commented Dec 6, 2020

Hold on a sec, I meant to see why the assert is there, and why it is in that form, and why it is IA32_ONLY. It should either be a generic assert (if it is good assert), or removed completely (if it is a superfluous assert). Code history should reveal the answer to that...

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Dec 7, 2020

/test

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 7, 2020

@DamonFool you need to get approval to run the tests in tier1 for commits up until 633034a

Loading

@openjdk openjdk bot added the test-request label Dec 7, 2020
@shipilev
Copy link
Contributor

@shipilev shipilev commented Dec 7, 2020

@DamonFool, I don't think /test works here, or even makes sense: no current testing runs Zero. Please try to make bootcycle-images on platforms you can reach.

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Dec 7, 2020

@DamonFool, I don't think /test works here, or even makes sense: no current testing runs Zero. Please try to make bootcycle-images on platforms you can reach.

'make bootcycle-images' have been done on Linux/86_32,64 and MacOS.
All passed.

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Dec 7, 2020

Hold on a sec, I meant to see why the assert is there, and why it is in that form, and why it is IA32_ONLY. It should either be a generic assert (if it is good assert), or removed completely (if it is a superfluous assert). Code history should reveal the answer to that...

The assert seems to be there at the creation of the OpenJDK repo.
It was checked in at the second commit:

commit 8153779ad32d1e8ddd37ced826c76c7aafc61894
Author: J. Duke <duke@openjdk.org>
Date:   Sat Dec 1 00:00:00 2007 +0000

    Initial load

I had checked the debug version of jdk8's zero on Linx/x86_32 finding that it crashed due to the same reason.
Maybe, the debug version of zero for x86_32 was broken at the very beginning.

Loading

@shipilev
Copy link
Contributor

@shipilev shipilev commented Dec 7, 2020

All right, this makes more sense to me. @dholmes-ora, could you do us a favor and see why this line: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp#L422

...was added? We don't have access to pre-OpenJDK history to get it ourselves.

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 7, 2020

Mailing list message from David Holmes on hotspot-runtime-dev:

On 7/12/2020 5:59 pm, Aleksey Shipilev wrote:

On Mon, 7 Dec 2020 07:38:03 GMT, Jie Fu <jiefu at openjdk.org> wrote:

Hold on a sec, I meant to see why the assert is there, and why it is in that form, and why it is `IA32_ONLY`. It should either be a generic assert (if it is good assert), or removed completely (if it is a superfluous assert). Code history should reveal the answer to that...

Hold on a sec, I meant to see why the assert is there, and why it is in that form, and why it is `IA32_ONLY`. It should either be a generic assert (if it is good assert), or removed completely (if it is a superfluous assert). Code history should reveal the answer to that...

The assert seems to be there at the creation of the OpenJDK repo.
It was checked in at the second commit:
commit 8153779
Author: J. Duke <duke at openjdk.org>
Date: Sat Dec 1 00:00:00 2007 +0000

 Initial load

I had checked the debug version of jdk8's zero on Linx/x86_32 finding that it crashed due to the same reason.
Maybe, the debug version of zero for x86_32 was broken at the very beginning.

All right, this makes more sense to me. @dholmes-ora, could you do us a favor and see why this line: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp#L422

...was added? We don't have access to pre-OpenJDK history to get it ourselves.

It came in as part of:

https://bugs.openjdk.java.net/browse/JDK-6571248

but there is no additional information.

Cheers,
David

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Dec 7, 2020

Mailing list message from David Holmes on hotspot-runtime-dev:

On 7/12/2020 5:59 pm, Aleksey Shipilev wrote:

On Mon, 7 Dec 2020 07:38:03 GMT, Jie Fu wrote:

Hold on a sec, I meant to see why the assert is there, and why it is in that form, and why it is IA32_ONLY. It should either be a generic assert (if it is good assert), or removed completely (if it is a superfluous assert). Code history should reveal the answer to that...

Hold on a sec, I meant to see why the assert is there, and why it is in that form, and why it is IA32_ONLY. It should either be a generic assert (if it is good assert), or removed completely (if it is a superfluous assert). Code history should reveal the answer to that...

The assert seems to be there at the creation of the OpenJDK repo.
It was checked in at the second commit:
commit 8153779
Author: J. Duke
Date: Sat Dec 1 00:00:00 2007 +0000

 Initial load

I had checked the debug version of jdk8's zero on Linx/x86_32 finding that it crashed due to the same reason.
Maybe, the debug version of zero for x86_32 was broken at the very beginning.

All right, this makes more sense to me. @dholmes-ora, could you do us a favor and see why this line: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp#L422
...was added? We don't have access to pre-OpenJDK history to get it ourselves.

It came in as part of:

https://bugs.openjdk.java.net/browse/JDK-6571248

but there is no additional information.

Cheers,
David

I will have a look.
Thanks.

Loading

Copy link
Contributor

@shipilev shipilev left a comment

After looking closer, I think this assert is not very useful, it is already disabled on all platforms except x86_32, and it is probably a leftover after many bulk Zero integrations. I think it is fine, safer, faster to remove it for all platforms. Do it, and I'll approve.

Loading

@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Dec 8, 2020

After looking closer, I think this assert is not very useful, it is already disabled on all platforms except x86_32, and it is probably a leftover after many bulk Zero integrations. I think it is fine, safer, faster to remove it for all platforms. Do it, and I'll approve.

Yes, I agree.
Done. Thanks.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 8, 2020

@DamonFool 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:

8257794: Zero: assert(istate->_stack_limit == istate->_thread->last_Java_sp() + 1) failed: wrong on Linux/x86_32

Reviewed-by: shade

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 37 new commits pushed to the master branch:

  • 1d0adbb: 8253644: C2: assert(skeleton_predicate_has_opaque(iff)) failed: unexpected
  • 51ac376: 8256411: Based anonymous classes have a weird end position
  • 0b6b6eb: 8257813: [redo] C2: Filter type in PhiNode::Value() for induction variables of trip-counted integer loops
  • 500ab45: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305
  • 6ff18e3: 8257855: Example SafeVarargsNotApplicableToRecordAccessors breaks test tools/javac/diags/CheckExamples.java
  • cef606f: 8253762: JFR: getField(String) should be able to access subfields
  • 39b8a2e: 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks
  • c43c224: 8257796: [TESTBUG] TestUseSHA512IntrinsicsOptionOnSupportedCPU.java fails on x86_32
  • 62c7788: 8257211: C2: Enable call devirtualization during post-parse phase
  • 149a02f: 8257572: Deprecate the archaic signal-chaining interfaces: sigset and signal
  • ... and 27 more: https://git.openjdk.java.net/jdk/compare/e590618962edc0d4d21ac05f813257bcef0a867f...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Dec 8, 2020
@DamonFool
Copy link
Member Author

@DamonFool DamonFool commented Dec 9, 2020

/integrate

Loading

@openjdk openjdk bot closed this Dec 9, 2020
@openjdk openjdk bot added the integrated label Dec 9, 2020
@openjdk openjdk bot removed ready rfr labels Dec 9, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 9, 2020

@DamonFool Since your change was applied there have been 58 commits pushed to the master branch:

  • fae7961: 8257884: Re-enable sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java as automatic test
  • 79f1dfb: 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException
  • 9ce3d80: 8257887: java/foreign/TestSegments.java test fails on 32-bit after JDK-8257186
  • 10da767: 8257847: Tiered should publish MDO data pointer for interpreter after profile start
  • 2a62d5d: 8256917: Use combo @returns tag in java.compiler javadoc
  • b29f9cd: 8075778: Add javadoc tag to avoid duplication of return information in simple situations.
  • 48d8650: 8257845: Integrate JEP 390
  • ed4c4ee: 8256299: Implement JEP 396: Strongly Encapsulate JDK Internals by Default
  • c47ab5f: 8256515: javax.xml.XMLEventReader produces incorrect START_DOCUMENT event
  • 291ba97: 8251267: CDS tests should use CDSTestUtils.getOutputDir instead of System.getProperty("user.dir")
  • ... and 48 more: https://git.openjdk.java.net/jdk/compare/e590618962edc0d4d21ac05f813257bcef0a867f...master

Your commit was automatically rebased without conflicts.

Pushed as commit df55ecd.

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

Loading

@DamonFool DamonFool deleted the JDK-8257794 branch Dec 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants