[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-dev
Subject:    Re: this-pointer NULL-checks in hotspot codebase [-Wtautological-undefined-compare]
From:       coleen.phillimore () oracle ! com
Date:       2019-07-29 15:16:42
Message-ID: f0d9965d-7284-83ce-4a40-6d0a9f94a176 () oracle ! com
[Download RAW message or body]



On 7/26/19 4:18 PM, Stefan Karlsson wrote:
> 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/ 
>

I didn't click all of it, but you could also move markOop* to markWord.* 
but keep it in oops, please.
>
> 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.

I vote for you to continue this!
Coleen
>
> 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
>>>>
>>
>

[prev in list] [next in list] [prev in thread] [next in thread]