Related lore: https://github.com/openjdk/valhalla/pull/1540 & 
https://github.com/openjdk/valhalla/pull/1751. Please, go check those up if you 
miss the context.

As we established in 
[JDK-8367151](https://bugs.openjdk.org/browse/JDK-8367151)/https://github.com/openjdk/valhalla/pull/1751,
 LR2 and FP2 are not reliable (resp. not patched for deopt and not known by 
deopt code, not updated by GC). Since reading them is probably fine, but maybe 
not, it is risky to leave reasonable value there. In debug, I suggest we store 
a magic but recognizable value to make more obvious one read the wrong copy, 
actually, we don't really need LR2 and FP2 to contain lr and rfp, we mostly 
need it to make space between the stack extension and the proper frame to 
pretend it is like a scalarized call.

What I propose here is similar to zapping unused space freed by the GC: when 
`ZapUnusedHeapArea`, that is `trueInDebug`, we zap the heap not to read 
something good-looking when we have a wrong pointer.

https://github.com/openjdk/valhalla/blob/1144cb4c5183c69a74aa0211f7ead5ac388ee41d/src/hotspot/share/runtime/globals.hpp#L482-L483

https://github.com/openjdk/valhalla/blob/1144cb4c5183c69a74aa0211f7ead5ac388ee41d/src/hotspot/share/gc/serial/serialFullGC.cpp#L371-L373

What I'm not sure about:
- should I make the `save_fake_rfp_lr` an argument also in product build, just 
unused, to avoid the slightly ugly `NOT_PRODUCT(COMMA save_fake_rfp_lr)`?
- how should I name `save_fake_rfp_lr`? I think it is clear, but not great.
- I've introduced a new value to zap registers, that looks special, but that is 
not what `badHeapWord` to avoid confusion. Any opinion on the variable name and 
the magic value? I intend to reuse it to zap other registers (the caller-saved 
ones).
- is there an easier way to write a 64-bit immediate in a register in Aarch64?! 
I found movptr, but it asserts the immediate is an address and so, that it is 
actually only 48-bits. I've wrote my own, because I couldn't find another 
example pointing me to an existing implementation of that, but I've probably 
missed something.

I've also elected not to make a flag but just to make mandatory to write these 
magic value in debug mode. I don't think it's worth a flag, as I see little 
benefit in not doing it: the performance cost is surely very marginal. Also, 
adding a flag, even develop, also implies some commitment (might end up in some 
tests or scripts), make sure it works to turn it on and off... Not terrible 
complications, but still a useless overhead.

And as proof it works:
- before [JDK-8367151](https://bugs.openjdk.org/browse/JDK-8367151) it makes 
`CorrectlyRestoreRfp.java` crash quickly
- now, if one breakpoints in `CorrectlyRestoreRfp.java` just before the call to 
`LargeValueWithOops.verify` in `LargeValueWithOops.compile_me_C2_verify`:
  
https://github.com/openjdk/valhalla/blob/1144cb4c5183c69a74aa0211f7ead5ac388ee41d/test/hotspot/jtreg/compiler/valhalla/inlinetypes/CorrectlyRestoreRfp.java#L95-L97
  one can see a stack like:

(rr) x/16xg $sp
0xffff7dea3f40: 0x0000ffff7dea3fa0      0x0000ffffa453d398
0xffff7dea3f50: 0x000000061c269230      0x0000ffffa4acb364
0xffff7dea3f60: 0x0000000000000002      0x0000ffff94906390
0xffff7dea3f70: 0x00000006bd264f28      0xfffffffffffffff2
0xffff7dea3f80: 0x0000ffff94907c78      0x0000000000000070   <-- [...] | sp_inc
0xffff7dea3f90: 0xcafefadecafefade      0xcafefadecafefade   <-- my fake 
values, taking space of FP2/LR2
0xffff7dea3fa0: 0x0000000000000000      0x0000000000000050   <-- the stack 
extension
0xffff7dea3fb0: 0x000000061c269230      0x0000ffff9d0002fc   <-- an OOP in rfp 
| the return address


Thanks,
Marc

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

Commit messages:
 - Not in product
 - Assert doesn't make sense anymore
 - Save fake values instead of lr/rfp

Changes: https://git.openjdk.org/valhalla/pull/1764/files
  Webrev: https://webrevs.openjdk.org/?repo=valhalla&pr=1764&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8371993
  Stats: 52 lines in 6 files changed: 25 ins; 16 del; 11 mod
  Patch: https://git.openjdk.org/valhalla/pull/1764.diff
  Fetch: git fetch https://git.openjdk.org/valhalla.git pull/1764/head:pull/1764

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

Reply via email to