On Tue, 14 Oct 2025 09:08:33 GMT, Marc Chevalier <[email protected]> wrote:
>> When regenerating >> `test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestMismatchHandling.jcod`, >> the preload attribute are back, after being removed in [8325660: [lworld] >> Update C2 to support new value construction scheme from JEP >> 401](https://bugs.openjdk.org/browse/JDK-8325660). This change basically >> disabled the test >> `test/hotspot/jtreg/compiler/valhalla/inlinetypes/TestMismatchHandling.java`. >> It is not quite clear why the test broke in between, but it doesn't work >> now! It seems there are two problems. >> >> The symptom is a wrong execution: we get a null pointer exception, when the >> pointer is clearly not null. The setup is around a call where the callee >> takes a value object as parameter (non-receiver), but the method happens to >> be mismatch, as detailed in [8301007: [lworld] Handle mismatches of the >> preload attribute in the calling >> convention](https://bugs.openjdk.org/browse/JDK-8301007). The caller is >> C2-compiled, the callee is interpreted. >> >> The caller is correctly compiled to pass a pointer to the callee, but the >> adapter is expecting a scalar convention, and interpret everything wrong, >> leading to the wrong execution. >> >> First problem is that optimized virtual calls are wrongly expected to never >> use the non-scalar convention: >> https://github.com/openjdk/valhalla/blob/lworld/src/hotspot/share/runtime/sharedRuntime.cpp#L1374-L1376 >> >> This fixes the original problems, but create a lot more! Well, just flavor >> of the same thing. >> >> They all come from piggybacking on the `evol_method` dependency that is used >> for JVMTI. This have various side effects that makes the code fail >> assertions a bit everywhere. Overall, dependencies coming from breakpoints >> are confused with some coming from mismatch calling convention, and some >> functions are used in both context, but not all. For instance (I might be a >> blurry on the details), it happens that a function is marked as having a >> mismatch calling convention, but later, some JVMTI related code will read >> the dependency as the existence of breakpoints (or something related), and >> refuse to compile it, making the test fail with >> `AbortVMOnCompilationFailure`. Distinguishing the cases becomes too >> complicated: while we can probably tell whether we added the dependency for >> JVMTI- or convention-related reasons, it is painful to propagate what we are >> looking for down the chain of calls. The best, and simplest, way is to >> introduce a new kind of dependency for calling convention mismatc h. It mostly behaves live the `evol_met... > > src/hotspot/share/code/nmethod.hpp line 1066: > >> 1064: // Used for fast breakpoint support if only_calling_convention is >> false; >> 1065: // used for updating the calling convention if true. >> 1066: bool is_dependent_on_method(Method* dependee, bool >> only_calling_convention); > > I'm not really happy about this `bool only_calling_convention`. I'd rather > like a `Dependencies::DepType` instead because it is only used in > > Dependencies::DepType dep_type = only_calling_convention ? > Dependencies::mismatch_calling_convention : Dependencies::evol_method; > > > The problem is that then I get a cyclic include between `nmethod.hpp` and > `dependencies.hpp`. It's probably avoidable, but I need to refactor a bit too > intensely than I feel comfortable in such a small fix. I guess an alternative would be to add separate methods, similar to what we have for `CodeCache::mark_dependents_on_method_for_breakpoint` -> `CodeCache::mark_dependents_on_method_for_mismatch` or something. That would at least limit the `only_calling_convention` arg to `is_dependent_on_method`. Or what about always checking both dependencies in `nmethod::is_dependent_on_method`? After all, both dependencies represent an actual dependency: - If the nmethod has a `evol_method` dependency, it's supposed to be deopted anyway. It doesn't matter where this happens. - If the nmethod has a `mismatch_calling_convention` dependency, in the worst case we deopt it when we reach this code via `CodeCache::mark_dependents_on_method_for_breakpoint` or `WB_DeoptimizeMethod`, i.e. when we want to make sure that all compiled versions (via inlining) of a method are deopted. So we would unnecessarily deopt the caller of a mismatched method when we set a breakpoint in that mismatched method (or deopt it via the WB API). I think that's fine. ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/1677#discussion_r2431344991
