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.

Sign up for GitHub

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

Jump to bottom

Convert last slash in hidden/anonymous class signatures to period #16100

Merged
merged 1 commit into from
Oct 21, 2022
Merged

Convert last slash in hidden/anonymous class signatures to period #16100

merged 1 commit into from
Oct 21, 2022

Conversation

ehrenjulzert
Copy link

Solves #15984

Signed-off-by: Ehren Julien-Neitzert [email protected]

@ehrenjulzert
Copy link
Author

@hangshao0

hangshao0
hangshao0 reviewed Oct 13, 2022
View reviewed changes
runtime/jvmti/jvmtiClass.c Outdated
if (J9ROMCLASS_IS_ANON_OR_HIDDEN(leafType->romClass)) {
int i = allocSize - 1;
for (; i >= 0; i--) {
if (signature[i] == '/') {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use ANON_CLASSNAME_CHARACTER_SEPARATOR instead of /.

The class name is generated from:

openj9/runtime/bcutil/ROMClassBuilder.cpp

Lines 408 to 415 in e8abb75

memcpy (constantPool[newUtfCPEntry].bytes + newHostPackageLength, originalStringBytes, originalStringLength);
*(U_8*)((UDATA) constantPool[newUtfCPEntry].bytes + newHostPackageLength + originalStringLength) = ANON_CLASSNAME_CHARACTER_SEPARATOR;
/*
* 0x<romAddress> will be appended to anon/hidden class name.
* Initialize the 0x<romAddress> part to 0x00000000 or 0x0000000000000000 (depending on the platforms).
*/
j9str_printf(PORTLIB, buf, ROM_ADDRESS_LENGTH + 1, ROM_ADDRESS_FORMAT, 0);
memcpy(constantPool[newUtfCPEntry].bytes + newHostPackageLength + originalStringLength + 1, buf, ROM_ADDRESS_LENGTH + 1);

You may want to add an additional check it is 0x<address> after the last ANON_CLASSNAME_CHARACTER_SEPARATOR.

Copy link
Author

Choose a reason for hiding this comment

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

updated in a95d7f9

hangshao0
hangshao0 reviewed Oct 17, 2022
View reviewed changes
runtime/jvmti/jvmtiClass.c Outdated
if (J9ROMCLASS_IS_ANON_OR_HIDDEN(leafType->romClass)) {
int i = allocSize - 3;
for (; i >= 0; i--) {
if ((signature[i] == ANON_CLASSNAME_CHARACTER_SEPARATOR) && (signature[i+1] == '0') && (signature[i+2] == 'x')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Format: please put the rvalue on the left side of the comparisons and put each comparison in its own line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment saying that the hidden/anon class names are generated from ROMClassBuilder::handleAnonClassName() and they have 0x<romaddress> appended at the end.

Copy link
Author

Choose a reason for hiding this comment

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

updated in 841425c

hangshao0
hangshao0 reviewed Oct 17, 2022
View reviewed changes
runtime/jvmti/jvmtiClass.c Outdated
int i = allocSize - 3;
for (; i >= 0; i--) {
/* hidden/anon class names are generated from ROMClassBuilder::handleAnonClassName() and have 0x<romaddress> appended at the end */
if ((ANON_CLASSNAME_CHARACTER_SEPARATOR == signature[i]) && ('0' == signature[i+1]) && ('x' == signature[i+2])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you split the comparisons into 3 lines, i.e.:

if ((ANON_CLASSNAME_CHARACTER_SEPARATOR == signature[i]) 
    && ('0' == signature[i+1]) 
    && ('x' == signature[i+2])
) {

Copy link
Author

Choose a reason for hiding this comment

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

updated in 841425c

@hangshao0 hangshao0 requested a review from tajila October 17, 2022 18:45
tajila
tajila reviewed Oct 19, 2022
View reviewed changes
runtime/jvmti/jvmtiClass.c Outdated
@@ -366,13 +366,29 @@ jvmtiGetClassSignature(jvmtiEnv* env,
signature[arity] = 'L';
memcpy(signature + arity + 1, J9UTF8_DATA(utf), utfLength);
signature[allocSize - 1] = ';';

if (J9ROMCLASS_IS_ANON_OR_HIDDEN(leafType->romClass)) {
int i = allocSize - 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

use IDATA instead of int

Copy link
Contributor

Choose a reason for hiding this comment

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

if ';' is at - 1 you can probably start from - 4

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, do we need to scan at all? doesnt ROM_ADDRESS_LENGTH indicate where the rom address starts?

Copy link
Contributor

Choose a reason for hiding this comment

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

we should make a helper for this

Copy link
Author

Choose a reason for hiding this comment

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

Good point, I'll use ROM_ADDRESS_LENGTH instead of scanning

Copy link
Author

Choose a reason for hiding this comment

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

updated in 3a22916

@tajila
Copy link
Contributor

tajila commented Oct 21, 2022

Jenkins test sanity win jdk8

@tajila
Copy link
Contributor

tajila commented Oct 21, 2022

jenkins test sanity amac jdk19

@tajila tajila merged commit 1af0a29 into eclipse-openj9:master Oct 21, 2022
@babsingh babsingh mentioned this pull request Nov 3, 2022
Closed
@pshipton pshipton mentioned this pull request Jan 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hangshao0 hangshao0 hangshao0 approved these changes

@tajila tajila tajila approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ehrenjulzert @tajila @hangshao0