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

Bengt Rutisson bengt.rutisson at
Thu Apr 9 16:48:21 UTC 2015

Hi Jon,

Can you explain more about the problem? I don't really follow the issue 

In the original code we have:

static void assert_all_threads_claimed() PRODUCT_RETURN;

  #ifndef PRODUCT
  void Threads::assert_all_threads_claimed() {
#endif // PRODUCT

StrongRootsScope::~StrongRootsScope() {

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?

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? In that case a different approach would be to 
make Thread::oops_do_parity() be PRODUCT_RETURN. Something like:

diff --git a/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 
--- 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) 

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. The thread.cpp/hpp files are just like all other 
hotspot files - rather inconsistent in their use of PRODUCT and ASSERT.


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> 
>> wrote:
>>> 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.
>>> 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.
> Jon

More information about the hotspot-gc-dev mailing list