RFR: 8361433: [Big Endian] assert(verify_guards()) failed: Expected valid memory guards after 8357601 [v2]

[David Holmes]

On Sat, 5 Jul 2025 17:30:58 GMT, Martin Doerr <mdoerr at openjdk.org> wrote:

>> src/hotspot/share/memory/guardedMemory.hpp line 122:
>> 
>>> 120:         // SafeFetch. It doesn't matter if the value read happens
>>> 121:         // to be 0xFF as that is not what we expect anyway.
>>> 122:         u_char val = (u_char) (SafeFetch32((int*)c, 0xFF) BIG_ENDIAN_ONLY(>> 24));
>> 
>> Your fix is fine. Pre-existing: I wonder about the SafeFetch. A byte pointer traverses byte-wise over the memory. We use it to load a 32-bit value via SafeFetch32. 3/4 of those loads will be unaligned and also redundant. (I am surprised the unaligned access works on all platforms).
>> 
>> The guards themselves can be located at unaligned addresses I think, so we cannot just use int loads. Therefore I would have just used the old code without safefetch but preceded it with a safefetch check:
>> 
>> 
>> u_char* c = (u_char*) _guard;
>> + if (!os::is_readable_pointer(align_up(c, BytesPerInt))) {
>> +   return false;
>> + }
>>    u_char* end = c + GUARD_SIZE;
>>    while (c < end) {
>>      if (*c != badResourceValue) {
>>        return false;
>>      }
>>      c++;
>>    }
>> 
>> I think that would be pragmatic since the guard is only 16 byte and the chance of a guard straddling a page boundary is very small. But if one wanted to be more careful, one could use `os::is_readable_range()` instead.
>> 
>> @dholmes-ora  does this make sense?
>
> Thanks, Thomas! I've changed to use `os::is_readable_range()`. Note that alignment is already handled inside of that function. Please take a look!

@TheRealMDoerr , @tstuefe  let me do a backout of JDK-8357601 in the morning. I need that fix to be complete and safe so it can be backported in a simple manner. Thanks.

I did not know that SafeFetch32 was not a generally usable API. I will look into @tstuefe suggested alternative.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/26140#discussion_r2188216085