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

-agentpath: libraries require no path/name decoration #20202

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JasonFengJ9
Copy link
Member

@JasonFengJ9 JasonFengJ9 commented Sep 20, 2024

-agentpath: libraries require no path/name decoration

The agent library specified via -agentpath: has an absolute path/name, it should not be decorated again during the agent library loading;
For -agentpath: option in the restore option file, compare the actual platform-dependent library name;
Added isCheckpointAllowed_VM()/isDebugOnRestoreEnabled_VM(), and refactored recent CRIU debug support related usages.

Related

closes #20194

Signed-off-by: Jason Feng [email protected]

@JasonFengJ9 JasonFengJ9 added comp:vm criu Used to track CRIU snapshot related work labels Sep 20, 2024
Comment on lines 5159 to +5160
BOOLEAN (*isCheckpointAllowed)(struct J9VMThread *currentThread);
BOOLEAN (*isCheckpointAllowed_VM)(struct J9JavaVM *vm);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need or want both isCheckpointAllowed and isCheckpointAllowed_VM?
Same question for isDebugOnRestoreEnabled / isDebugOnRestoreEnabled_VM.

Copy link
Member Author

Choose a reason for hiding this comment

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

isCheckpointAllowed() and isDebugOnRestoreEnabled() already have lots of usages, this PR only refactored the usage by recent #19754, I would leave others in a separate PR instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you think the switch to the _VM signatures should be done separately, I suggest it should be done first (to avoid the unnecessary duplication).

Copy link
Member Author

@JasonFengJ9 JasonFengJ9 Sep 20, 2024

Choose a reason for hiding this comment

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

Opened

@dsouzai suggested holding it until #20047 is merged.

This PR is a follow-up of #19754, and is required for 0.48.

Existing code has a similar pattern

BOOLEAN
isCRaCorCRIUSupportEnabled(J9VMThread *currentThread)
{
return isCRaCorCRIUSupportEnabled_VM(currentThread->javaVM);
}
BOOLEAN
isCRaCorCRIUSupportEnabled_VM(J9JavaVM *vm)
{
return J9_IS_CRIU_OR_CRAC_CHECKPOINT_ENABLED(vm);
}

Can this PR be merged first before the refactoring PR #20205?

The agent library specified via -agentpath: has an absolute path/name,
it should not be decorated again during the agent library loading;
For -agentpath: option in the restore option file, compare the actual
platform-dependent library name;
Added isCheckpointAllowed_VM()/isDebugOnRestoreEnabled_VM(), and
refactored recent CRIU debug support related usages.

Signed-off-by: Jason Feng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:vm criu Used to track CRIU snapshot related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

80 FVT_Serviceability.diagnostics.xdump_thrstart.Mode101.1 Initialization error for library j9jvmti29(-3)
2 participants