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

Fix padding in jvmtiCapabilities structure #15451

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

EricYangIBM
Copy link
Contributor

JvmtiCapabilities struct cannot have padding between fields.

Signed-off-by: Eric Yang eric.yang@ibm.com

@EricYangIBM
Copy link
Contributor Author

EricYangIBM commented Jun 28, 2022

Jdk19 before and after:

	unsigned int can_generate_resource_exhaustion_threads_events : 1;
	unsigned int can_generate_early_vmstart : 1;
	unsigned int can_generate_early_class_hook_events : 1;
	unsigned int can_generate_sampled_object_alloc_events : 1;
	unsigned int : 4;
	unsigned int can_support_virtual_threads : 1;
	unsigned int : 15;
	unsigned int : 16;
	unsigned int : 16;
	unsigned int : 16;
	unsigned int : 16;
} jvmtiCapabilities;
	unsigned int can_generate_resource_exhaustion_threads_events : 1;
	unsigned int can_generate_early_vmstart : 1;
	unsigned int can_generate_early_class_hook_events : 1;
	unsigned int can_generate_sampled_object_alloc_events : 1;
	unsigned int can_support_virtual_threads : 1;
	unsigned int : 3;
	unsigned int : 16;
	unsigned int : 16;
	unsigned int : 16;
	unsigned int : 16;
	unsigned int : 16;
} jvmtiCapabilities;

@EricYangIBM
Copy link
Contributor Author

@tajila @babsingh I could also change it to use compound conditions.
Currently:

if (JAVA_SPEC_VERSION >= 9) {
	unsigned int can_generate_early_vmstart : 1;
	unsigned int can_generate_early_class_hook_events : 1;
	if (JAVA_SPEC_VERSION >= 11) {
		unsigned int can_generate_sampled_object_alloc_events : 1;
		if (JAVA_SPEC_VERSION >= 19) {
			unsigned int can_support_virtual_threads : 1;
			unsigned int : 3;
		} else {
			unsigned int : 4;
		}
	} else {
		unsigned int : 5;
	}
} else {
	unsigned int : 7;
}

Alternative:

if (JAVA_SPEC_VERSION >= 9 && JAVA_SPEC_VERSION < 11) {
	unsigned int can_generate_early_vmstart : 1;
	unsigned int can_generate_early_class_hook_events : 1;
	unsigned int : 5;
} else if (JAVA_SPEC_VERSION >= 11 && JAVA_SPEC_VERSION < 19) {
	unsigned int can_generate_early_vmstart : 1;
	unsigned int can_generate_early_class_hook_events : 1;
	unsigned int can_generate_sampled_object_alloc_events : 1;
	unsigned int : 4;
} else if (JAVA_SPEC_VERSION >= 19) {
	unsigned int can_generate_early_vmstart : 1;
	unsigned int can_generate_early_class_hook_events : 1;
	unsigned int can_generate_sampled_object_alloc_events : 1;
	unsigned int can_support_virtual_threads : 1;
	unsigned int : 3;
} else {
	unsigned int : 7;
}

Is this better?

@babsingh
Copy link
Contributor

babsingh commented Jun 28, 2022

re #15451 (comment):

IMO, the alternative is easier to understand. So, +1 for the alternative. The current nested version can lead to errors due to lack of simplicity. Will the below also work; it avoids duplicates?

if (JAVA_SPEC_VERSION >= 9) {
	unsigned int can_generate_early_vmstart : 1;
	unsigned int can_generate_early_class_hook_events : 1;
}
if (JAVA_SPEC_VERSION >= 11 ) {
	unsigned int can_generate_sampled_object_alloc_events : 1;
}
if (JAVA_SPEC_VERSION >= 19 ) {
	unsigned int can_support_virtual_threads : 1;
}
/* Add a comment about PADDING:
 * There will be errors if PADDING is wrong, and
 * it needs to be updated whenever a new field is added.
 */
if (JAVA_SPEC_VERSION >= 9 && JAVA_SPEC_VERSION < 11) {
	unsigned int : 5;
} else if (JAVA_SPEC_VERSION >= 11 && JAVA_SPEC_VERSION < 19) {
	unsigned int : 4;
} else if (JAVA_SPEC_VERSION >= 19) {
	unsigned int : 3;
} else {
	/* PADDING for Java 8. */
	unsigned int : 7;
}

@EricYangIBM
Copy link
Contributor Author

EricYangIBM commented Jun 29, 2022

Will the below also work; it avoids duplicates?

I think this is more confusing than the first alternative
Edit - the above updated version looks good

@babsingh
Copy link
Contributor

In the first alternative, the duplicates are a drawback. @tajila What are your thoughts?

runtime/include/jvmti.h.m4 Outdated Show resolved Hide resolved
JvmtiCapabilities struct cannot have padding between fields.

Signed-off-by: Eric Yang <eric.yang@ibm.com>
@tajila
Copy link
Contributor

tajila commented Jul 6, 2022

I generally like to avoid duplication, so I dont mind the nesting too much

@EricYangIBM
Copy link
Contributor Author

So this pr is fine to be merged?

@tajila
Copy link
Contributor

tajila commented Jul 8, 2022

jenkins test sanity amac jdk19

@tajila
Copy link
Contributor

tajila commented Jul 8, 2022

jenkins compile win jdk19

@tajila
Copy link
Contributor

tajila commented Jul 8, 2022

only test failure is known issue

@tajila tajila merged commit 6b4a2db into eclipse-openj9:master Jul 8, 2022
@EricYangIBM EricYangIBM deleted the fix_jvmtiCapabilities branch July 8, 2022 17:01
@babsingh babsingh mentioned this pull request Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants