Request for Review (xs) - 8077301: Optimized build is broken

Fri Apr 10 17:34:16 UTC 2015


On 4/10/2015 12:27 AM, Bengt Rutisson wrote:
>
> Hi Jon,
>
> Thanks for confirming that I understood the problem correctly. I think 
> you make a valid point of moving the compilation boundary up rather 
> than down as I suggested. So, your latest webrev looks good to me. 
> Reviewed.
>
Thanks for the review.

> Just for the record I think it is unfortunate that NOT_DEBUG_RETURN 
> isn't called NOT_ASSERT_RETURN. The confusion around the PRODUCT and 
> ASSERT defines is enough without having the extra concept of "debug". 
> (Similarly DEBUG_ONLY, debug_only and NOT_DEBUG are badly named in my 
> opinion.)
YES INDEED!  I think the original thinking was that all things DEBUG 
could go into
debug builds and be as slow as needed.  All things ASSERT would go into 
fastdebug
and be nimble.  But the "fast" part of fastdebug died from a thousand 
cuts so the
distinction is not very meaningful anymore.

Jon

>
> Thanks,
> Bengt
>
> On 2015-04-10 00:05, Jon Masamitsu wrote:
>> Bengt,
>>
>> On 4/9/2015 9:48 AM, Bengt Rutisson wrote:
>>>
>>> Hi Jon,
>>>
>>> Can you explain more about the problem? I don't really follow the 
>>> issue here.
>>>
>>> In the original code we have:
>>>
>>> thread.hpp:
>>> static void assert_all_threads_claimed() PRODUCT_RETURN;
>>>
>>> thread.cpp:
>>>  #ifndef PRODUCT
>>>  void Threads::assert_all_threads_claimed() {
>>>  ...
>>> #endif // PRODUCT
>>>
>>> strongRootsScope.cpp:
>>> StrongRootsScope::~StrongRootsScope() {
>>>   Threads::assert_all_threads_claimed();
>>> }
>>>
>>>
>>> It seems like it is all using the same guard (PRODUCT)? Why is this 
>>> a problem for optimized builds? I thought optimized was that neither 
>>> PRODUCT nor ASSERT is defined.  So, I would expect an optimized 
>>> build to use the implementation in thread.cpp. Why doesn't it find 
>>> that?
>>
>> You mean that an optimized build would use
>>
>> #ifndef PRODUCT
>> void Threads::assert_all_threads_claimed() {
>>   ALL_JAVA_THREADS(p) {
>>     const int thread_parity = p->oops_do_parity();
>>     assert((thread_parity == _thread_claim_parity),
>>         err_msg("Thread " PTR_FORMAT " has incorrect parity %d != 
>> %d", p2i(p), thread_parity, _thread_claim_parity));
>>   }
>> }
>> #endif // PRODUCT
>>
>> which it does.   Which has the oops_do_parity() method call which you 
>> discuss below.
>>
>>>
>>>
>>> I think the issue is that Threads::assert_all_threads_claimed() 
>>> calls Thread::oops_do_parity(), which is only defined for ASSERT 
>>> builds. Is that the real problem? 
>>
>> Yes, that's the root problem.
>>
>>> In that case a different approach would be to make 
>>> Thread::oops_do_parity() be PRODUCT_RETURN. Something like:
>>
>> What you suggest below works as you say.   I like drawing the 
>> compile-in or NOT compile-in
>> box around assert_all_threads_claimed() just because the name makes 
>> it so clear to me that
>> it should only be compiled-in for builds were I expect assertion 
>> checking to be done.
>>>
>>> diff --git a/src/share/vm/runtime/thread.cpp 
>>> b/src/share/vm/runtime/thread.cpp
>>> --- a/src/share/vm/runtime/thread.cpp
>>> +++ b/src/share/vm/runtime/thread.cpp
>>> @@ -843,2 +843,6 @@
>>>
>>> +int Thread::oops_do_parity() const {
>>> +  return _oops_do_parity;
>>> +}
>>> +
>>>  // The flag: potential_vm_operation notifies if this particular 
>>> safepoint state could potential
>>> diff --git a/src/share/vm/runtime/thread.hpp 
>>> b/src/share/vm/runtime/thread.hpp
>>> --- a/src/share/vm/runtime/thread.hpp
>>> +++ b/src/share/vm/runtime/thread.hpp
>>> @@ -553,3 +553,2 @@
>>>    bool owns_locks_but_compiled_lock() const;
>>> -  int oops_do_parity() const                     { return 
>>> _oops_do_parity; }
>>>
>>> @@ -561,2 +560,4 @@
>>>
>>> +  int oops_do_parity() const PRODUCT_RETURN;
>>> +
>>>    void check_for_valid_safepoint_state(bool potential_vm_operation) 
>>> PRODUCT_RETURN;
>>
>> oops_do_parity() is an accessor and it suffers from the inconsistency 
>> of using accessors.  I always
>> like to use them (inside the class where it could be accessed as a 
>> private variable as well as outside
>> the class where some public access is needed).  This is more in the 
>> eye of the beholder than most
>> but it looks wrong to me to have a PRODUCT_RETURN on an accessor.
>>
>>>
>>>
>>> I think I prefer this since it more directly addresses what the 
>>> problem is.
>>>
>>> With the above patch "optimized" builds on my linux machine at 
>>> least. I guess the argument could be made that all of this should be 
>>> guarded by ASSERT rather than PRODUCT, but that's a different issue 
>>> than making the optimized builds work. 
>> I think that the inconsistency of using #ifndef PRODUCT rather than 
>> #ifdef ASSERT is the root of
>> the problems with optimiized builds.
>>
>> As I said above the name "assert" in the name 
>> assert_all_threads_claimed() tells me that I
>> can define it under #ifdef ASSERT.  I like that but I'll willing to 
>> go the oops_do_parity() route
>> if you see more value in it.  Think about it and let me know.
>>
>> Thanks for looking at this.
>>
>> Jon
>>
>>
>>> The thread.cpp/hpp files are just like all other hotspot files - 
>>> rather inconsistent in their use of PRODUCT and ASSERT.
>>>
>>> Thanks,
>>> Bengt
>>>
>>>
>>>
>>> On 2015-04-09 18:18, Jon Masamitsu wrote:
>>>>
>>>> Kim,
>>>>
>>>> Thanks for looking at this.
>>>>
>>>> On 4/8/2015 10:26 PM, Kim Barrett wrote:
>>>>> On Apr 8, 2015, at 10:42 PM, Jon Masamitsu 
>>>>> <jon.masamitsu at oracle.com> wrote:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8077301
>>>>>>
>>>>>> The "optimized" target does not build.   A method is used in an 
>>>>>> assert
>>>>>> but is defined under #ifndef PRODUCT and should be defined under
>>>>>> #ifdef  ASSERT.
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jmasa/8077301/webrev.00/
>>>>>>
>>>>>> I built optimized, fastdebug, and product successfully after
>>>>>> the fix.
>>>>> This change seems inconsistent with the function declaration in 
>>>>> the header, which is
>>>>>
>>>>>    static void assert_all_threads_claimed() PRODUCT_RETURN;
>>>>>
>>>>> Maybe the declaration should be changed to NOT_DEBUG_RETURN too?
>>>>
>>>> Yes that makes more sense.  I've changed to NOT_DEBUG_RETURN.
>>>>>
>>>>> I’m also confused about how “optimized” would build with the 
>>>>> proposed change.  I thought
>>>>> “optimized” means neither PRODUCT nor ASSERT are defined. I see 
>>>>> how that would fail
>>>>> without the change, due to an unused variable warning, because the 
>>>>> variable is only used
>>>>> in an assert.  But with this change I would expect to see a 
>>>>> link-time error because the
>>>>> function is referenced but lacks a definition.
>>>>
>>>> You're right again.  I was overly confident that it would work once 
>>>> it compiled.  With the
>>>> change to use  NOT_DEBUG_RETURN, it builds and executes.
>>>>
>>>> Fixed webrev.
>>>>
>>>> http://cr.openjdk.java.net/~jmasa/8077301/webrev.01/
>>>>
>>>> Jon
>>>>
>>>>>
>>>>
>>>
>>
>