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

[Bengt Rutisson]

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.

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

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