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

[Martin Doerr]

On Mon, 23 Dec 2024 07:23:38 GMT, Thomas Stuefe <stuefe 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 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. My thought is: since we cannot hope to achieve portability for these platforms anyway, we might just as well allow adding to NULL. Casting feels like just switching off these warnings. 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.
> 
> 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.

Undefined behavior is a risk. Compilers currently seem to do what we want, but there's no guarantee. They could optimize the code in a way which breaks it. That would be legal since it is undefined behavior. `uintptr_t` arithmetics are defined as well as the casts. "Any value of type std::nullptr_t, including nullptr can be converted to any integral type as if it were (void*)0" [https://en.cppreference.com/w/cpp/language/reinterpret_cast]. I guess compilers need to handle the conversion during the casts on platforms on which nullptr is not the zero address.

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

PR Comment: https://git.openjdk.org/jdk/pull/22848#issuecomment-2559371631