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

[Bengt Rutisson]

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?


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


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.

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