this-pointer NULL-checks in hotspot codebase [-Wtautological-undefined-compare]

[Stefan Karlsson]

FWIW, I have a prototype that rewrites markOopDesc that gets rid of this 
undefined behavior. I got some internal feedback that it was a 
worth-while change, but I didn't have time to get this into JDK 13, and 
post-poned it.

The first patch renames the markOopDesc to MarkWord and removes the 
inheritances from oopDesc:
  https://cr.openjdk.java.net/~stefank/prototype/markWord/webrev.markWord.simpleRename/

On top of that I have the patch that makes MarkWord an AllStatic class 
and removes the UB:
  https://cr.openjdk.java.net/~stefank/prototype/markWord/webrev.markWord.makeStatic.delta/
  https://cr.openjdk.java.net/~stefank/prototype/markWord/webrev.markWord.makeStatic/

This was written in May and hasn't been rebased against the latest changes.

StefanK

On 2019-07-12 16:46, Erik Österlund wrote:
> Hi Harold,
>
> It's worse than that though, unfortunately. You are not allowed to
> have "this" equal to NULL, whether you perform such explicit NULL
> comparisons or not.
>
> The implication is that as long as "inflating" is NULL, we kind of
> can't use any of the functions on markOop and hence  mustrewrite
> pretty much all uses of markOop to do something else.
> The same goes for things like Register, where rax == NULL. To be
> compliant, we would similarly have to rewrite all uses of Register.
>
> In other words, if we are to really hunt down uses of this == NULL
> and remove them, we will find ourselves with a mountain of work.
>
> Again, just gonna drop that here and run.
>
> /Erik
>
> On 2019-07-12 14:14, Harold Seigel wrote:
>> The functions that compare 'this' to NULL could be changed from 
>> instance to static functions where 'this' is explicitly passed as a 
>> parameter.  Then you could keep the equivalent NULL checks.
>>
>> Harold
>>
>> On 7/12/2019 4:22 AM, Erik Österlund wrote:
>>> Hi Matthias,
>>>
>>> Removing such NULL checks seems like a good idea in general due to 
>>> the undefined behaviour.
>>> Worth mentioning though that there are some tricky ones, like in 
>>> markOopDesc* where this == NULL
>>> means that the mark word has the "inflating" value. So we explicitly 
>>> check if this == NULL and
>>> hope the compiler will not elide the check. Just gonna drop that one 
>>> here and run for it.
>>>
>>> Thanks,
>>> /Erik
>>>
>>> On 2019-07-12 09:48, Baesken, Matthias wrote:
>>>> Hello , when looking  into  the recent xlc16 / xlclang   warnings I 
>>>> came  across  those  3 :
>>>>
>>>> /nightly/jdk/src/hotspot/share/adlc/formssel.cpp:1729:7: warning: 
>>>> 'this' pointer cannot be null in well-defined C++ code;
>>>> comparison may be assumed to always evaluate to true 
>>>> [-Wtautological-undefined-compare]
>>>>    if( this != NULL ) {
>>>>        ^~~~    ~~~~
>>>>
>>>> /nightly/jdk/src/hotspot/share/adlc/formssel.cpp:3416:7: warning: 
>>>> 'this' pointer cannot be null in well-defined C++ code;
>>>> comparison may be assumed to always evaluate to false 
>>>> [-Wtautological-undefined-compare]
>>>>    if( this == NULL ) return;
>>>>
>>>> /nightly/jdk/src/hotspot/share/libadt/set.cpp:46:7: warning: 'this' 
>>>> pointer cannot be null in well-defined C++ code;
>>>> comparison may be assumed to always evaluate to false 
>>>> [-Wtautological-undefined-compare]
>>>>    if( this == NULL ) return os::strdup("{no set}");
>>>>
>>>>
>>>> Do you think the  NULL-checks can be removed or is there still some 
>>>> value in doing them ?
>>>>
>>>> Best regards, Matthias
>>>
>