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

8312495: assert(0 <= i && i < _len) failed: illegal index after JDK-8287061 on big endian platforms #14976

Conversation

reinrich
Copy link
Member

@reinrich reinrich commented Jul 21, 2023

On big endian platforms jint values are stored in the high part of StackValue values. Therefore the the StackValue cannot be cast directly to jint. More details why this has to be like this are given in the JBS issue.

This is a common pattern. See also

val = value->get_int();
obj->int_at_put(index, (jint)*((jint*)&val));

val = value->get_int();
obj->int_field_put(offset, (jint)*((jint*)&val));

Testing

Manny iterations of vmTestbase/vm/mlvm/meth/stress/compiler/sequences/Test.java.

JTReg tests: tier1-4 of hotspot and jdk. All of Langtools and jaxp. Renaissance benchmarks as functional tests.

All testing was done with fastdebug and release builds on the main platforms and also on Linux/PPC64le.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8312495: assert(0 <= i && i < _len) failed: illegal index after JDK-8287061 on big endian platforms (Bug - P2)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14976/head:pull/14976
$ git checkout pull/14976

Update a local copy of the PR:
$ git checkout pull/14976
$ git pull https://git.openjdk.org/jdk.git pull/14976/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14976

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14976.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 21, 2023

👋 Welcome back rrich! 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 8312495: assert(0 <= i && i < _len) failed: illegal index after JDK-8287061 on big endian platforms 8312495: assert(0 <= i && i < _len) failed: illegal index after JDK-8287061 on big endian platforms Jul 21, 2023
@openjdk
Copy link

openjdk bot commented Jul 21, 2023

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

  • hotspot-compiler

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-compiler hotspot-compiler-dev@openjdk.org label Jul 21, 2023
@reinrich reinrich marked this pull request as ready for review July 21, 2023 17:01
@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 21, 2023
@mlbridge
Copy link

mlbridge bot commented Jul 21, 2023

Webrevs

Copy link
Contributor

@RealCLanger RealCLanger 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. Minor formatting comment.

// On big endian platforms the jint is in the high part of the StackValue
intptr_t val = sv_selector->get_int();
jint selector = (jint)*((jint*)&val);

Copy link
Contributor

Choose a reason for hiding this comment

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

not necessary to add a new line here

@openjdk
Copy link

openjdk bot commented Jul 21, 2023

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

8312495: assert(0 <= i && i < _len) failed: illegal index after JDK-8287061 on big endian platforms

Reviewed-by: clanger, kvn, dlong

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

  • 486c784: 8312433: HttpClient request fails due to connection being considered idle and closed
  • 271417a: 8312579: [JVMCI] JVMCI support for virtual Vector API objects
  • 44576a7: 8312466: /bin/nm usage in AIX makes needs -X64 flag
  • 86821a7: 8312235: [JVMCI] ConstantPool should not force eager resolution
  • 7cbab1f: 8312218: Print additional debug information when hitting assert(in_hash)
  • 01e135c: 8312440: assert(cast != nullptr) failed: must have added a cast to pin the node
  • b7545a6: 8313164: src/java.desktop/windows/native/libawt/windows/awt_Robot.cpp GetRGBPixels adjust releasing of resources
  • 36d578c: 8311653: Modify -XshowSettings launcher behavior
  • a9d21c6: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074
  • 4c2e54f: 8309088: security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java fails
  • ... and 107 more: https://git.openjdk.org/jdk/compare/a53345ad03e07ab2a990721a506ebc25eed0f7c9...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jul 21, 2023
@dean-long
Copy link
Member

How about introducing something like jint Value::get_jint() that can be used in place of this error-prone pattern?

jint selector = sv_selector->get_int();
// On big endian platforms the jint is in the high part of the StackValue
intptr_t val = sv_selector->get_int();
jint selector = (jint)*((jint*)&val);
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need outer cast but you may need () around (&val). See the example:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/stackValueCollection.cpp#L29

@vnkozlov
Copy link
Contributor

How about introducing something like jint Value::get_jint() that can be used in place of this error-prone pattern?

And may be rename current method get_int() to get_intptr() to match type of return value.

@reinrich
Copy link
Member Author

Thanks for the feedback!

How about introducing something like jint Value::get_jint() that can be used in place of this error-prone pattern?

And may be rename current method get_int() to get_intptr() to match type of return value.

I agree. I thought it should be done in a dedicated RFE. But I can do it in this PR.

@dean-long
Copy link
Member

I agree. I thought it should be done in a dedicated RFE. But I can do it in this PR.

It seems reasonable to me to allow small enhancements along with bug fixes. If it's a separate RFE then you have to either push that first, or do a temporary bug fix that doesn't use the enhancement. What are the chances that we would want to back-port one without the other?

@reinrich
Copy link
Member Author

I've done the the refactoring. There are a few places were I wasn't sure what to do:

  • differences in the handling of Location::dbl and lng in reassign_fields_by_klass and reassign_type_array_elements
  • Should the logic of StackValueCollection::float_at and similar be moved to StackValue?
  • what to do with byte_array_put?
  • Should get_jint be implemented as a simple (jint) cast of _integer_value on little endian platforms?

@vnkozlov
Copy link
Contributor

I've done the the refactoring. There are a few places were I wasn't sure what to do:

Good.

  • differences in the handling of Location::dbl and lng in reassign_fields_by_klass and reassign_type_array_elements

What difference you talking about?

  • Should the logic of StackValueCollection::float_at and similar be moved to StackValue?

I think this is the case for separate RFE.

  • what to do with byte_array_put?

Explain please

  • Should get_jint be implemented as a simple (jint) cast of _integer_value on little endian platforms?

No. The code you suggest works for all case - there is no need to complicate it.

@reinrich
Copy link
Member Author

The langtools/tier1 failure on linux-x86 looks unrelated

#  Internal Error (g1ConcurrentMark.cpp:1671), pid=37061, tid=37071
#  fatal error: Overflow during reference processing, can not continue. Current mark stack depth: 65472, MarkStackSize: 65536, MarkStackSizeMax: 4194304. Please increase MarkStackSize and/or MarkStackSizeMax and restart.

@reinrich
Copy link
Member Author

  • differences in the handling of Location::dbl and lng in reassign_fields_by_klass and reassign_type_array_elements

What difference you talking about?

reassign_fields_by_klass falls through to the T_LONG and T_DOUBLE case

where the intptr is fed as-is into a simple long_field_put

obj->long_field_put(offset, res);

reassign_type_array_elements extracts the halfs from the intptr value and stores them separately

obj->int_at_put(index, (jint)*((jint*)&res));
obj->int_at_put(++index, (jint)*(((jint*)&res) + 1));

Instead of the 2 stores with the complex casts just one long_at_put could be done. Or is there a reason not to do that?

  • Should the logic of StackValueCollection::float_at and similar be moved to StackValue?

I think this is the case for separate RFE.

Ok.

  • what to do with byte_array_put?

Explain please

Well, it has very similar patterns like the locations I changed so far. I guess it is better to leave it alone for now.

  • Should get_jint be implemented as a simple (jint) cast of _integer_value on little endian platforms?

No. The code you suggest works for all case - there is no need to complicate it.

Sure.

@vnkozlov
Copy link
Contributor

The langtools/tier1 failure on linux-x86 looks unrelated

#  Internal Error (g1ConcurrentMark.cpp:1671), pid=37061, tid=37071
#  fatal error: Overflow during reference processing, can not continue. Current mark stack depth: 65472, MarkStackSize: 65536, MarkStackSizeMax: 4194304. Please increase MarkStackSize and/or MarkStackSizeMax and restart.

https://bugs.openjdk.org/browse/JDK-8312534

@vnkozlov
Copy link
Contributor

  • differences in the handling of Location::dbl and lng in reassign_fields_by_klass and reassign_type_array_elements

What difference you talking about?

So you are asking about difference for case T_INT: case T_FLOAT: and not case T_LONG: case T_DOUBLE:.
Let leave it as it is for now and file RFE to investigate (to factor out and use the same code).

@vnkozlov
Copy link
Contributor

what to do with byte_array_put?
Explain please
Well, it has very similar patterns like the locations I changed so far. I guess it is better to leave it alone for now.

It is not big change - just pass StackValue* argument.

@reinrich
Copy link
Member Author

  • differences in the handling of Location::dbl and lng in reassign_fields_by_klass and reassign_type_array_elements

What difference you talking about?

So you are asking about difference for case T_INT: case T_FLOAT: and not case T_LONG: case T_DOUBLE:. Let leave it as it is for now and file RFE to investigate (to factor out and use the same code).

Ok. I've created https://bugs.openjdk.org/browse/JDK-8312753

@reinrich
Copy link
Member Author

Tests succeeded with the latest version.
@vnkozlov @dean-long are you ok with the refactoring?
I'll be out of office next week. It would be good if big endian platforms could be fixed before.
If in doubt I would do the minimal fix and postpone the refactoring.

Copy link
Contributor

@vnkozlov vnkozlov 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. Let me run our internal testing for latest version before approval.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

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

My testing passed clean.

@reinrich
Copy link
Member Author

My testing passed clean.

Thanks for testing and thanks to everybody helping with this pr.

/integrate

@openjdk
Copy link

openjdk bot commented Jul 27, 2023

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

  • 486c784: 8312433: HttpClient request fails due to connection being considered idle and closed
  • 271417a: 8312579: [JVMCI] JVMCI support for virtual Vector API objects
  • 44576a7: 8312466: /bin/nm usage in AIX makes needs -X64 flag
  • 86821a7: 8312235: [JVMCI] ConstantPool should not force eager resolution
  • 7cbab1f: 8312218: Print additional debug information when hitting assert(in_hash)
  • 01e135c: 8312440: assert(cast != nullptr) failed: must have added a cast to pin the node
  • b7545a6: 8313164: src/java.desktop/windows/native/libawt/windows/awt_Robot.cpp GetRGBPixels adjust releasing of resources
  • 36d578c: 8311653: Modify -XshowSettings launcher behavior
  • a9d21c6: 8313081: MonitoringSupport_lock should be unconditionally initialized after 8304074
  • 4c2e54f: 8309088: security/infra/java/security/cert/CertPathValidator/certification/AmazonCA.java fails
  • ... and 107 more: https://git.openjdk.org/jdk/compare/a53345ad03e07ab2a990721a506ebc25eed0f7c9...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jul 27, 2023
@openjdk openjdk bot closed this Jul 27, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 27, 2023
@openjdk
Copy link

openjdk bot commented Jul 27, 2023

@reinrich Pushed as commit 8661b8e.

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

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