On Tue, 14 Nov 2023 17:56:09 GMT, Jaroslav Bachorik <jbacho...@openjdk.org> wrote:
> Please, review this fix for a corner case handling of `jmethodID` values. > > The issue is related to the interplay between `jmethodID` values and method > redefinitions. Each `jmethodID` value is effectively a pointer to a `Method` > instance. Once that method gets redefined, the `jmethodID` is updated to > point to the last `Method` version. > Unless the method is still on stack/running, in which case the original > `jmethodID` will be redirected to the latest `Method` version and at the same > time the 'previous' `Method` version will receive a new `jmethodID` pointing > to that previous version. > > If we happen to capture stacktrace via `GetStackTrace` or `GetAllStackTraces` > JVMTI calls while this previous `Method` version is still on stack we will > have the corresponding frame identified by a `jmethodID` pointing to that > version. > However, sooner or later the 'previous' class version becomes eligible for > cleanup at what time all contained `Method` instances. The cleanup process > will not perform the `jmethodID` pointer maintenance and we will end up with > pointers to deallocated memory. > This is caused by the fact that the `jmethodID` lifecycle is bound to > `ClassLoaderData` instance and all relevant `jmethodID`s will get > batch-updated when the class loader is being released and all its classes are > getting unloaded. > > This means that we need to make sure that if a `Method` instance is being > deallocate the associated `jmethodID` (if any) must not point to the > deallocated instance once we are finished. Unfortunately, we can not just > update the `jmethodID` values in bulk when purging an old class version - the > per `InstanceKlass` jmethodID cache is present only for the main class > version and contains `jmethodID` values for both the old and current method > versions. > > Therefore we need to perform `jmethodID` lookup when we are about to > deallocate a `Method` instance and clean up the pointer only if that > `jmethodID` is pointing to the `Method` instance which is being deallocated. > > _(For anyone interested, a much lengthier writeup is available in [my > blog](https://jbachorik.github.io/posts/mysterious-jmethodid))_ src/hotspot/share/oops/instanceKlass.cpp line 531: > 529: > 530: void InstanceKlass::deallocate_methods(ClassLoaderData* loader_data, > 531: Array<Method*>* methods, > InstanceKlass* klass) { An explicit boolean parameter would be cleaner/clearer. src/hotspot/share/oops/instanceKlass.cpp line 542: > 540: if (klass) { > 541: jmethodID jmid = method->find_jmethod_id_or_null(); > 542: // Do the pointer maintenance before releasing the metadata, > just in case I assume there should be a period after 'case`. But just in case of what? src/hotspot/share/oops/instanceKlass.cpp line 549: > 547: if (jmid != nullptr && *((Method**)jmid) == method) { > 548: *((Method**)jmid) = nullptr; > 549: } This should be abstracted behind a utility function e.g. `method->clear_jmethod_id()`. src/hotspot/share/oops/method.cpp line 2277: > 2275: } > 2276: } > 2277: Can this race with redefinition? src/hotspot/share/oops/method.hpp line 730: > 728: // so handles are not used to avoid deadlock. > 729: jmethodID find_jmethod_id_or_null() { > 730: return method_holder() != nullptr ? > method_holder()->jmethod_id_or_null(this) : nullptr; If `method_holder()` is null at this point what does that mean for the lifecycle of the Method? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1393663791 PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1393664277 PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1393672300 PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1395072721 PR Review Comment: https://git.openjdk.org/jdk/pull/16662#discussion_r1395071297
