Skip to content

Add safeguard for J9-specific jmethodid unloading behaviour (version-dependent) #164

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

Merged
MattAlp merged 2 commits into from
Jan 8, 2025
Merged

Add safeguard for J9-specific jmethodid unloading behaviour (version-dependent) #164

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
391fb43
Add safeguard for J9-specific jmethodid unloading behaviour (version-…
Jan 7, 2025
9125860
Added comments providing context on J9 crash safeguards
Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion ddprof-lib/src/main/cpp/flightRecorder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,14 @@ void Lookup::fillJavaMethodInfo(MethodInfo *mi, jmethodID method,
bool entry = false;
if (VMMethod::check_jmethodID(method) &&
jvmti->GetMethodDeclaringClass(method, &method_class) == 0 &&
jvmti->GetClassSignature(method_class, &class_name, NULL) == 0 &&
// On some older versions of J9, the JVMTI call to GetMethodDeclaringClass will return OK = 0, but when a
// classloader is unloaded they free all JNIIDs. This means that anyone holding on to a jmethodID is
// pointing to corrupt data and the behaviour is undefined.
// The behaviour is adjusted so that when asgct() is used or if `-XX:+KeepJNIIDs` is specified,
// when a classloader is unloaded, the jmethodIDs are not freed, but instead marked as -1.
// The nested check below is to mitigate these crashes.
// In more recent versions, the condition above will short-circuit safely.
((!VM::isOpenJ9() || method_class != reinterpret_cast<jclass>(-1)) && jvmti->GetClassSignature(method_class, &class_name, NULL) == 0) &&
Copy link
Contributor

@r1viollet r1viollet Jan 9, 2025

Choose a reason for hiding this comment

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

minor / style: we could have declared the -1 value as a constexpr value (k_invalid_j9_method_class ?)
Is checking the -1 value not enough (to avoid the isOpenJ9 call) ?

Copy link
Collaborator

@jbachorik jbachorik Jan 9, 2025

Choose a reason for hiding this comment

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

Hm, now when rereading the comment, this does not make much sense.
The info from J9 engineers is about invalid jmethodid becoming -1 - but that's not what we were seeing, right? It was the jclass returned from GetMethodDeclaringClass on such invalid jmethodid that was -1.
I mean, the workaround would work but the root cause seems to be J9 not handling the invalid jmethodids consistently in the JVMTI calls itself (eg. GetMethodDeclaringClass when called on an invalid jmethodid should return an error code instead of putting -1 to a jclass variable 🤷

Copy link
Contributor Author

@MattAlp MattAlp Jan 10, 2025

Choose a reason for hiding this comment

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

the root cause seems to be J9 not handling the invalid jmethodids consistently in the JVMTI calls itself

I agree with this. I believe that this fixed in later versions of J9, but is considered acceptable for others (hence version-dependent in the PR title). It would be more accurate to do a specific J9 version check here rather than simply !VM::isOpenJ9(), but that opens up the possibility of us missing some versions in our known-to-be-buggy list.

Copy link
Contributor Author

@MattAlp MattAlp Jan 10, 2025

Choose a reason for hiding this comment

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

@r1viollet

  1. I'll constexpr, it doesn't hurt
  2. I checked isOpenJ9() earlier - it returns a boolean based on a member that is set once at profiler startup, so I think the call is negligible. I added the check prior to adding the comments for the sake of self-documenting code, but also to remove the comparison for non-J9 VMs. WDYT - remove it?

java-profiler/ddprof-lib/src/main/cpp/vmEntry.h

Line 169 in 82a3272

static bool isOpenJ9() { return _openj9; }

Copy link
Contributor

@r1viollet r1viollet Jan 15, 2025

Choose a reason for hiding this comment

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

  1. As a general guidance we would want to avoid reinterpret casts when possible. The constexpr part is just a good habit to take (though sometimes hard to apply if we use old C++ versions)
  2. The j9 call is more a question of taste and obviously very minor.

jvmti->GetMethodName(method, &method_name, &method_sig, NULL) == 0) {

if (first_time) {
Expand Down
Loading