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

[Jon Masamitsu]

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