RFR: 8346714: [ASAN] compressedKlass.cpp reported applying non-zero offset to null pointer [v2]

[Andrew Haley]

On 12/23/24 07:26, Thomas Stuefe wrote:
> On Sun, 22 Dec 2024 17:46:52 GMT, Andrew Haley <aph at openjdk.org> wrote:
> 
>>>> Thanks for fixing the issue! This should work. In general, I still prefer using `uintptr_t` because `intptr_t` has undefined behavior on overflow. Probably not in this case, here.
>>>
>>>
>>> +1. Also, instead of casting manually, please use p2u (and if there is none, we should add it like we have a p2i for intptr_t).
>>> I am somewhat unenthusiastic about changes like these. We seem to add a lot of casting boilerplate to satisfy UBsan for the sake of cleanliness that only matters on special hardware that treats pointers differently from numeric.
>>
>> True, but we can very effectively make the compiler and the reader happy by using `uintptr_t` for all of the arithmetic. After all, we are not using null-based compressed pointers, we are using zero-based compressed pointers, and the distinction is important. `uintptr_t` does not have null as a member, it has zero; conversely `address` does not have zero, it has null. Therefore, when we are using zero-based compressed pointers, better to to use `uintptr_t` for the arithmetic.
> 
> Okay, but this is just for the sake of the reader, right? Because a cast can be just as invalid as adding to NULL.

I don't agree.  Adding to NULL is explicitly UB. C++ doesn't have to generate
any code for it.

> I worked on hardware that marked pointers as invalid if they resulted from an integer cast. I assume that "don't add to NULL" warnings exist to preserve portability for these platforms.

That's not so. Current C++ compilers can, and some do, correctly generate no code for anything following UB.

If you were to convert a pointer to uintptr_t, discover that it fits in an int, convert it to an int, and
then convert it back to a pointer, that is well-defined C++, and must be executed. You can even write a
pointer to a file, read it back later, and use that pointer. And it must work.

> My thought is: since we cannot hope to achieve portability for these platforms anyway, we might just as well allow adding to NULL.

No. That is UB.

> Casting feels like just switching off these warnings.

That's how it feels, sure, but you're moving from UB to implementation-defined behaviour.
The latter we can cope with.

> Note that even if I convert the whole of CompressedKlassPointers to uintptr_t, we will need a cast at the latest when we use the base as input to mmap.

Yes. Good.

> But I agree that UBSAN cleanliness is valuable since most of its warnings are good, and maybe its just simpler to fix these issues than to argue about them. So, I am fine with this patch.