On Thu, 12 Sep 2024 09:22:04 GMT, Dean Long <dl...@openjdk.org> wrote:
>> Matias Saavedra Silva has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Coleen suggestion > > I'm trying to determine if this PR changes anything in regards to compilers, > or leaves everything the same, so I am looking at callers of get_new_method() > that don't already map is_deleted() to throw_no_such_method_error(). > Is it true that CallInfo::resolved_method() and CallInfo::selected_method() > can never return an is_old or is_deleted method? They check for is_old but > not is_deleted. If get_new_method() actually returned nullptr for a deleted > method then some callers might crash, so I am assuming this is impossible. > There is probably a rule that if you have a resolved CallInfo then you aren't > allowed to safepoint, so there is no way the resolved/selected methods in the > CallInfo can change to old or deleted, and it's probably impossible for them > start out as old/deleted before a safepoint. So why are these methods > checking for is_old at all? @dean-long tbh I don't know what method_orig_idnum means without doing more research on it. It was added for a special case that I'd have to dig up. The idnum is what matches its entry in the jmethodID array and the matching in method_with_idnum() provides that we find the method with the equivalent signature. If you don't add or delete methods, redefinition can still reorder the methods because they are only sorted by name, and not name and signature. idnum is the right thing to use. It is true and _required_ that CallInfo never return an old method. We used to have more is_old() checks through the compiler code and CallInfo is the surface that we now check for this. In at least one place, we have a NoSafepointVerifier. See JDK-8327250 for more details in that bug description. In the interpreter, we don't have a NSV but the redefinition will replace the old methods in the cpCache so running in the interpreter will get the new version of the method. So back to worry that the no-arguments for the throw_NSME() replacement, could break things. The vtables and itables will never have this, as we cannot delete virtual methods. Before Matias's patch, we could return nullptr to some callers from CallInfo, and that will definitely crash. It's probably hard to construct a test case to show this, but maybe possible. The compiler has to store a deleted method in the CI or nmethod somewhere. If we have NSME on the stack and do a GC and the arguments aren't collected but not used (but consumed because we clear the expression stack (?)) could this cause a problem? For PopFrame, I don't know how to PopFrame from Unsafe::NSME method. If this is a problem, we would have to artificially add methods with the same signature to call NSME like we do for default_methods processing. This is a large undertaking and we should fix this bug first and if we can prove that this isn't sufficient add this code, we should file an but to study this because I don't know the answer to that. This change was to fix the null pointer problem at the source, which I think is the first step. ------------- PR Comment: https://git.openjdk.org/jdk/pull/20874#issuecomment-2346222646
