RFR: 8320275: assert(_chunk->bitmap().at(index)) failed: Bit not set at index

[Patricio Chilano Mateo]

On Tue, 28 Nov 2023 00:09:10 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Please review the following fix. The assert fails while verifying the top frame of the stackChunk before returning from a thaw call. The stackChunk is in gc mode but we found a narrow oop for this c2 compiled frame that doesn't have its corresponding bit set. This is because while thawing its callee we cleared the bitmap range associated with the argument area, but this narrow oop happens to land at the very last stack slot of that region.
> Loom code assumes the size of the argument area is always a multiple of 2 stack slots, as SharedRuntime::java_calling_convention() shows. But c2 doesn't seem to follow this convention and, knowing the last passed argument only takes one stack slot, it's using the remaining space to store a narrow oop for the caller. There are more details about the specific crash in JBS.
> 
> The initial proposed fix is to just restrict the range of the bitmap we clear by excluding the last stack slot of the argument area, since passed oops are always word aligned. I've also experimented with a patch where I changed SharedRuntime::java_calling_convention() and Fingerprinter::do_type_calling_convention() to not round up the number of stack slots used, and then changed the callers to use a round up value or not depending on the needs [1]. I wasn't convinced it was worthy given we only care about this difference in this Loom code, but I don't mind going with that fix instead. The 3rd alternative would be to just change c2 to not use this stack slot and start spilling at a word aligned offset from the sp.
> 
> I run the patch with the failing test and verified the crash doesn't reproduce anymore. I've also run this patch through loom tiers1-5. 
> 
> Thanks,
> Patricio
> 
> [1] https://github.com/pchilano/jdk/commit/42ae9269b28beb6f36c502182116545b680e418f

Running some extra tests I see the callee can use the argument area to store data that is different from the one passed. This is actually something @fparain told me some time ago. So this simpler solution won't do. Before applying https://github.com/pchilano/jdk/commit/42ae9269b28beb6f36c502182116545b680e418f instead, @dean-long  how about if we just prevent c2 from using this stack slot for the caller?

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

PR Comment: https://git.openjdk.org/jdk/pull/16837#issuecomment-1829909893