[lworld] RFR: 8369185: [lworld] Wrong execution in TestMismatchHandling after regenerating TestMismatchHandling.jcod [v2]
Marc Chevalier
mchevalier at openjdk.org
Wed Oct 15 13:08:19 UTC 2025
On Wed, 15 Oct 2025 06:56:53 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:
>> 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.
I started with separate methods, but that was unfortunate duplication. I went for the second option, since testing seems happy. I'm not sure about the new names I introduced tho (`method_types` and `has_method_dep`). If you have better idea, I'll be happy to change them.
-------------
PR Review Comment: https://git.openjdk.org/valhalla/pull/1677#discussion_r2432489457
More information about the valhalla-dev
mailing list