[lworld] RFR: 8369185: [lworld] Wrong execution in TestMismatchHandling after regenerating TestMismatchHandling.jcod

[Tobias Hartmann]

On Tue, 14 Oct 2025 09:08:33 GMT, Marc Chevalier <mchevalier at openjdk.org> 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