On Thu, 27 Nov 2025 21:04:28 GMT, Marc Chevalier <[email protected]> wrote:
> 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 ... That is a very useful continuation of this story arch. > how should I name `save_fake_rfp_lr`? `mangle_unused_rfp_lr`, `zap_unextended_rfp_lr` and permutations of those come to mind. > is there an easier way to write a 64-bit immediate in a register in Aarch64? You could load a constant from the constant pool (`ldr xTmp, =imm64`) if you want to trade off code size vs. having a load from memory. Otherwise, I think that your version is the best you can do. src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5906: > 5904: if (framesize < ((1 << 9) + 2 * wordSize)) { > 5905: sub(sp, sp, framesize); > 5906: #ifndef PRODUCT Are you sure you want this in the `optimized` build? Since that is used for profiling performance, I would tend towards using `ASSERT`. src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5911: > 5909: movk(rscratch1, (badRegWordVal >> 16) & 0xffff, 16); > 5910: movk(rscratch1, badRegWordVal & 0xffff, 32); > 5911: movk(rscratch1, (badRegWordVal >> 16) & 0xffff, 48); This should go in its own function in the macro assembler. src/hotspot/share/utilities/globalDefinitions.hpp line 1047: > 1045: const intptr_t badDispHeaderDeopt = 0xDE0BD000; // value to > fill unused displaced header during deoptimization > 1046: const intptr_t badDispHeaderOSR = 0xDEAD05A0; // value to > fill unused displaced header during OSR > 1047: const juint badRegWordVal = 0xCAFEFADE; // value > used to zap registers Since you asked: keeping the `0xDEAD` prefix would make sense. Otherwise, I have no opinion, because there is no good way to spell "R" in hex. ------------- Changes requested by mhaessig (no project role). PR Review: https://git.openjdk.org/valhalla/pull/1764#pullrequestreview-3517919380 PR Review Comment: https://git.openjdk.org/valhalla/pull/1764#discussion_r2570810116 PR Review Comment: https://git.openjdk.org/valhalla/pull/1764#discussion_r2570816195 PR Review Comment: https://git.openjdk.org/valhalla/pull/1764#discussion_r2570805892
