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

[Thomas Stuefe]

On Sun, 22 Dec 2024 17:46:52 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> 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 wh...
>
>> > 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.

@theRealAph @TheRealMDoerr Thank you for your points! You have convinced me.

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

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