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

[Dean Long]

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

I don't really like the use of `address` for the pointer that might be either `oop` or `narrowOop`, because the bitmap can't really support an arbitrary `char*` address.  I think it would be better to cleanup methods that take `intptr_t*` and instead use `template <typename OopT>` like `bit_index_for`.
Also, I would prefer removing the round up in `java_calling_convention`, because the current patch fixes a subtle bug with an even subtler work-around, but let's see what other reviews think.

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

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