Skip to content

Fix LocalJ9UTF8Buffer initialization #19773

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

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

ThanHenderson
Copy link
Contributor

@ThanHenderson ThanHenderson commented Jun 27, 2024

This patch:

  • fixes the default constructor for LocalJ9UTF8Buffer by initializing its members
  • uses direct construction for LocalJ9UTF8Buffer instantiation
  • avoids full buffer initialization for static name and signature buffers
  • uses J9ArrayClass::arity rather than loops to determine array dimensions

Signed-off-by: Nathan Henderson [email protected]

@ThanHenderson
Copy link
Contributor Author

@keithc-ca This PR addresses your review comments from: #19648

@keithc-ca
Copy link
Contributor

I'm concerned that some logic errors crept into #19648; for example:

  • in getClassSignatureLength(), I don't think it properly computes the length for [J (or an array where the final element type is primitive) because it terminates the loop without counting the last level - see [1] and [2]
  • similarly, I don't see proper treatment of array types in getClassSignatureInout(): how do the right number of [ characters get added before [3]?

Should revert #19648 until we address those points? @babsingh @pshipton What do you think?

[1] https://github.com/eclipse-openj9/openj9/blob/master/runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp#L425
[2] https://github.com/eclipse-openj9/openj9/blob/master/runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp#L496
[3] https://github.com/eclipse-openj9/openj9/blob/master/runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp#L474

@ThanHenderson
Copy link
Contributor Author

I'm concerned that some logic errors crept into #19648

Since none of the logic you are referring to was changed in this patch, if I understand what you are getting at correctly, you mean: some existing logic errors crept into #19648, and that we should revert back, fix the existing errors, and re-apply this patch with the corrected logic.

@pshipton
Copy link
Member

If it's not a regression I don't see a need to revert.

@ThanHenderson
Copy link
Contributor Author

@fengxue-IS and I can audit the current code to see if there are any other missed cases.

@fengxue-IS
Copy link
Contributor

fengxue-IS commented Jul 3, 2024

@keithc-ca from what I recall, the skip on primitive is specifically done because one dimensional primitive arrays are recognized as a class in the VM. see https://github.com/eclipse-openj9/openj9/blob/master/runtime/vm/romclasses.c#L56-L64

I'm concerned that some logic errors crept into #19648; for example:

  • in getClassSignatureLength(), I don't think it properly computes the length for [J (or an array where the final element type is primitive) because it terminates the loop without counting the last level - see [1] and [2]

In this cases, any primitive array where it terminates the loop before counting the last level is because one dimensional primitive arrays are a special class in the VM. so the code will directly use the classname[length] of that class in https://github.com/eclipse-openj9/openj9/blob/master/runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp#L432-L433

so this actually computes the correct array classname by counting number of [ -1 and appending a classname length for of [J after. The same rule on primitive array applies for the getClassSignatureInout() function.

  • similarly, I don't see proper treatment of array types in getClassSignatureInout(): how do the right number of [ characters get added before [3]?

Should revert #19648 until we address those points? @babsingh @pshipton What do you think?

[1] https://github.com/eclipse-openj9/openj9/blob/master/runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp#L425 [2] https://github.com/eclipse-openj9/openj9/blob/master/runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp#L496 [3] https://github.com/eclipse-openj9/openj9/blob/master/runtime/jcl/common/java_lang_invoke_MethodHandleNatives.cpp#L474

@ThanHenderson
Copy link
Contributor Author

@keithc-ca do you have any concerns with the explanation in @fengxue-IS comment? Either way, could we merge this one?

@keithc-ca
Copy link
Contributor

The loop in getClassSignatureLength() could be removed by using J9ArrayClass::arity, like elsewhere (e.g. jclvm.c).

@ThanHenderson
Copy link
Contributor Author

@keithc-ca I've addressed your concerns. Let me know if you have any other suggestions.

This patch:
- fixes the default constructor for LocalJ9UTF8Buffer by initializing
  its members
- uses direct construction for LocalJ9UTF8Buffer instantiation
- avoids full buffer initialization for static name and signature
  buffers
- uses J9ArrayClass::arity rather than loops to determine array
  dimensions

Signed-off-by: Nathan Henderson <[email protected]>
@keithc-ca
Copy link
Contributor

Jenkins test sanity alinux jdk21

@keithc-ca keithc-ca merged commit 4e8d679 into eclipse-openj9:master Jul 10, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants