In `SharedRuntime::get_resolved_entry` when `current->is_interp_only_mode()` we 
need to go to the interpreter, and thus we must always return a c2i adapter 
(and not the entry point of another compiled method).

https://github.com/openjdk/valhalla/blob/d5e3620d5a0b13cce9c19bafd5b2dd881b8bf9e9/src/hotspot/share/runtime/sharedRuntime.cpp#L1635-L1641

Yet, here, we disregard the kind of call, or the caller. Yet, under, we make 
the correct distinction:

https://github.com/openjdk/valhalla/blob/d5e3620d5a0b13cce9c19bafd5b2dd881b8bf9e9/src/hotspot/share/runtime/sharedRuntime.cpp#L1643-L1652

Indeed, if the caller is C1, the call won't be scalarized, so we don't need the 
packing logic that would be required for a scalarized call from C2, for 
instance... Overall, we must distinguish the 3 same cases as when 
`is_interp_only_mode` is not set. The only difference is that instead of 
picking the compiled entry point if available, and only an adapter as a 
fallback, we must take the adapter. Right now, the code is correct only if 
`!caller_does_not_scalarize && (is_static_call || is_optimized)` (so, 
especially not calls from C1).

I've slightly changed the shape of the code to make that more clear, and also 
to be less likely to forget a case if we add new variations in the calling 
convention!

If this error doesn't show up so often, that's because `is_interp_only_mode()` 
is only set by JVMTI:

https://github.com/openjdk/valhalla/blob/d5e3620d5a0b13cce9c19bafd5b2dd881b8bf9e9/src/hotspot/share/prims/jvmtiThreadState.cpp#L322-L328
and
https://github.com/openjdk/valhalla/blob/d5e3620d5a0b13cce9c19bafd5b2dd881b8bf9e9/src/hotspot/share/prims/jvmtiThreadState.inline.hpp#L151-L153

So, without JVMTI, it is impossible to observe this bug, which is unfortunate. 
So, how to test that, not to do the same error again? A test involving JVMTI 
would be brittle, and we can do better than adding yet another test: making 
better usage of existing tests. Now, `StressCallingConvention` will sometimes 
make `get_resolved_entry` behaves as if `is_interp_only_mode` is set. Doing so 
causes SO MANY crashes in `TestCallingConventionC1`, which is expected. With 
the fix, it works again as expected.

The frequency of `is_interp_only_mode` being randomly true when stressing is 
currently set at a very arbitrary 1/2^10 which is around 0.1%. I've also tested 
with a probability of 1/2, and I get basically the same behavior: plenty of 
crashes with the improved stress without the fix, and no crashes with the fix.

Tested with 
`tier1,tier2,tier3,hs-precheckin-comp,hs-comp-stress,valhalla-comp-stress`.

Thanks,
Marc

-------------

Commit messages:
 - Fix

Changes: https://git.openjdk.org/valhalla/pull/2124/files
  Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=2124&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8378099
  Stats: 22 lines in 4 files changed: 16 ins; 1 del; 5 mod
  Patch: https://git.openjdk.org/valhalla/pull/2124.diff
  Fetch: git fetch https://git.openjdk.org/valhalla.git pull/2124/head:pull/2124

PR: https://git.openjdk.org/valhalla/pull/2124

Reply via email to