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

8248003: [lworld] [lw3] VM crashes when classes with inline type fields are loaded from CDS archive #95

Closed
wants to merge 4 commits into from

Conversation

@fparain
Copy link
Collaborator

@fparain fparain commented Jun 22, 2020

Please review this changeset fixing several issues in CDS related to inline type metadata removal and restoration.

Thank you,

Fred


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8248003: [lworld] [lw3] VM crashes when classes with inline type fields are loaded from CDS archive

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/95/head:pull/95
$ git checkout pull/95

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 22, 2020

👋 Welcome back fparain! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 22, 2020

@fparain This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

8248003: [lworld] [lw3] VM crashes when classes with inline type fields are loaded from CDS archive
  • If you would like to add a summary, use the /summary command.
  • To credit additional contributors, use the /contributor command.
  • To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 3 commits pushed to the lworld branch:

  • 45400b1: 8247923: [lworld] Empty inline types break calling convention optimization
  • d615a33: 8248167: [lworld] [lw3] JdbInlineTypesTest fails
  • e5eb253: 8247795: allow factory methods for inline types to return j.l.Obje…

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge lworld into your branch, and then specify the current head hash when integrating, like this: /integrate 45400b1ebd69691011223596543bce6acd9f1366.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 22, 2020

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 22, 2020

Mailing list message from Ioi Lam on valhalla-dev:

Hi Fred,

The changesets look OK to me. It is mixed with both CDS changes and
function renaming. I reviewed only the CDS changes.

instanceKlass.cpp

2666?? if (has_inline_type_fields()) {
2667???? for (AllFieldStream fs(fields(), constants()); !fs.done();
fs.next()) {
2668?????? if (Signature::basic_type(fs.signature()) == T_VALUETYPE) {
2669???????? reset_inline_type_field_klass(fs.index());
2670?????? }
2671???? }
2672?? }

systemDictionary.cpp:

1484?? if (ik->has_inline_type_fields()) {
1485???? for (AllFieldStream fs(ik->fields(), ik->constants());
!fs.done(); fs.next()) {
1486?????? if (Signature::basic_type(fs.signature()) == T_VALUETYPE) {
1487???????? if (!fs.access_flags().is_static()) {
1488?????????? // Pre-load inline class
1489?????????? Klass* real_k =
SystemDictionary::resolve_inline_type_field_or_fail(&fs,
1490???????????? class_loader, protection_domain, true, CHECK_NULL);
1491?????????? Klass* k =
ik->get_inline_type_field_klass_or_null(fs.index());
1492?????????? if (real_k != k) {
1493???????????? // oops, the app has substituted a different version of k!
1494???????????? return NULL;
1495?????????? }
1496???????? } else {
1497????????? ik->reset_inline_type_field_klass(fs.index());
1498???????? }

[1] Is line 1497 still necessary?
[2] For testing line 1494, I would recommend writing a new test case
that dynamically redefines a "Q" class. You can see an example:

test/hotspot/jtreg/runtime/cds/appcds/RewriteBytecodesTest.java

Thanks
- Ioi

On 6/22/20 5:56 AM, Frederic Parain wrote:

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 22, 2020

Mailing list message from Frederic Parain on valhalla-dev:

Ioi,

Thank you for reviewing these changes.
Line 1497 was added when I was trying to be more selective in
InstanceKlass::remove_unshareable_info() but it didn?t work out.
After unconditional reset at instanceKlass.cpp:2669, then
line 1497 in systemDictionary.cpp is not needed anymore. Thank you
for spotting this.

I?m looking at RewriteBytecodesTest.java.

Fred

On Jun 22, 2020, at 14:25, Ioi Lam <ioi.lam at oracle.com> wrote:

Hi Fred,

The changesets look OK to me. It is mixed with both CDS changes and function renaming. I reviewed only the CDS changes.

instanceKlass.cpp

2666 if (has_inline_type_fields()) {
2667 for (AllFieldStream fs(fields(), constants()); !fs.done(); fs.next()) {
2668 if (Signature::basic_type(fs.signature()) == T_VALUETYPE) {
2669 reset_inline_type_field_klass(fs.index());
2670 }
2671 }
2672 }

systemDictionary.cpp:

1484 if (ik->has_inline_type_fields()) {
1485 for (AllFieldStream fs(ik->fields(), ik->constants()); !fs.done(); fs.next()) {
1486 if (Signature::basic_type(fs.signature()) == T_VALUETYPE) {
1487 if (!fs.access_flags().is_static()) {
1488 // Pre-load inline class
1489 Klass* real_k = SystemDictionary::resolve_inline_type_field_or_fail(&fs,
1490 class_loader, protection_domain, true, CHECK_NULL);
1491 Klass* k = ik->get_inline_type_field_klass_or_null(fs.index());
1492 if (real_k != k) {
1493 // oops, the app has substituted a different version of k!
1494 return NULL;
1495 }
1496 } else {
1497 ik->reset_inline_type_field_klass(fs.index());
1498 }

[1] Is line 1497 still necessary?
[2] For testing line 1494, I would recommend writing a new test case that dynamically redefines a "Q" class. You can see an example:

test/hotspot/jtreg/runtime/cds/appcds/RewriteBytecodesTest.java

Thanks
- Ioi

On 6/22/20 5:56 AM, Frederic Parain wrote:

@fparain
Copy link
Collaborator Author

@fparain fparain commented Jun 23, 2020

The fix has been updated according to Ioi's review, and a new test has been added with class redefinition.

Thank you,

Fred

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 24, 2020

Mailing list message from Ioi Lam on valhalla-dev:

Hi Fred,

The changes look good to me. For this comment:

// Even if the Point class is not loaded from the CDS archive, make sure
the WithInlineField class
// can still be loaded successfully, and properly get the rewritten
version of Point.

[1] spelling of WithInlineField -> WithInlinedField
[2] I think you can add

// The archived version of WithInlinedField must not be loaded, because
it references the archived
// version of Point, but a different version of Point has been loaded.

Thanks
- Ioi

On 6/23/20 10:52 AM, Frederic Parain wrote:

@fparain
Copy link
Collaborator Author

@fparain fparain commented Jun 24, 2020

Ioi,

Thank you for the second review. I've updated the comment as you suggested.

Fred.

@fparain
Copy link
Collaborator Author

@fparain fparain commented Jun 24, 2020

/integrate

@openjdk openjdk bot closed this Jun 24, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 24, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Jun 24, 2020

@fparain The following commits have been pushed to lworld since your change was applied:

  • 45400b1: 8247923: [lworld] Empty inline types break calling convention optimization
  • d615a33: 8248167: [lworld] [lw3] JdbInlineTypesTest fails
  • e5eb253: 8247795: allow factory methods for inline types to return j.l.Obje…

Your commit was automatically rebased without conflicts.

Pushed as commit 3f8947b.

@fparain fparain deleted the fix_CDS branch Aug 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
1 participant