RFR: 8326936: RISC-V: Shenandoah GC crashes due to incorrect atomic memory operations [v4]

[MaxXing]

On Mon, 4 Mar 2024 07:43:03 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> MaxXing has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add comments, change `flag` to `rc_temp`.
>
> Hey,
> 
>> So the return value should be sign-extended, even if it's unsigned (according to my understand). Here's a simple example: https://godbolt.org/z/n6vfoYPPx. GCC generates `slliw` for the unsigned return value.
>> 
> 
> This method should, at least often, be inlined, so old_value will not be passed as a return value.
> And someone may add additional code in the same method.
> Now it seems like the compiler always sign extends every word from inline asm, but I don't think that is a guarantee?
> I was told always pass registers to inline asm and not types.
> So I suggest having the correct sign extended word type, e.g. int32_t, as old_value.
> 
>> > Anyhow can't we just use a int32_t when comparing, and cast that back to the 'uint32_t' on return? (and still use _atomic...)
>> 
>> Seems we can't. Check this example for more details: https://godbolt.org/z/cv5ejTfxh. I wrote both the inline assembly version and the built-in version CAS, and tested them on the same code snippet. The inline assembly version works well due to the ABI, but the built-in version is still buggy.
> 
> Yes, I don't find a good way to force it. It possible using volatile but it involves storing and load from stack.
> 
> Thanks, Robbin

Hi @robehn , Thanks for your suggestions!

> Now it seems like the compiler always sign extends every word from inline asm, but I don't think that is a guarantee?
> I was told always pass registers to inline asm and not types.
> So I suggest having the correct sign extended word type, e.g. int32_t, as old_value.

You are right. I tried the inline assembly version CAS on Clang (although we don't enable this patch for Clang), and the parameter don't seem to be sign-extended unless we force it: https://godbolt.org/z/fM8n9GMcz.

I added an explicit type cast for `compare_value`, and changed the type of `old_value` to `int32_t` in the latest commit.

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

PR Comment: https://git.openjdk.org/jdk/pull/18039#issuecomment-1977914750