openj9 icon indicating copy to clipboard operation
openj9 copied to clipboard

Fix LocalJ9UTF8Buffer initialization

Open ThanHenderson opened this issue 1 year ago • 4 comments

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

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

ThanHenderson avatar Jun 27 '24 19:06 ThanHenderson

@keithc-ca This PR addresses your review comments from: https://github.com/eclipse-openj9/openj9/pull/19648

ThanHenderson avatar Jun 27 '24 19:06 ThanHenderson

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

keithc-ca avatar Jun 28 '24 20:06 keithc-ca

I'm concerned that some logic errors crept into https://github.com/eclipse-openj9/openj9/pull/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 https://github.com/eclipse-openj9/openj9/pull/19648, and that we should revert back, fix the existing errors, and re-apply this patch with the corrected logic.

ThanHenderson avatar Jun 28 '24 21:06 ThanHenderson

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

pshipton avatar Jun 28 '24 21:06 pshipton

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

ThanHenderson avatar Jul 02 '24 23:07 ThanHenderson

@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

fengxue-IS avatar Jul 03 '24 05:07 fengxue-IS

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

ThanHenderson avatar Jul 08 '24 13:07 ThanHenderson

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

keithc-ca avatar Jul 08 '24 14:07 keithc-ca

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

ThanHenderson avatar Jul 08 '24 21:07 ThanHenderson

Jenkins test sanity alinux jdk21

keithc-ca avatar Jul 10 '24 13:07 keithc-ca