RFR: 8200550: Xcode 9.3 produce warning on heapRegionSet.hpp

[Kim Barrett]

> On Apr 9, 2018, at 12:01 PM, coleen.phillimore at oracle.com wrote:
> 
> 
> 
> On 4/9/18 2:55 AM, Kim Barrett wrote:
>>> On Apr 7, 2018, at 5:24 PM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>>> 
>>> Hi,
>>> 
>>> On Sat, 2018-04-07 at 15:48 -0400, Kim Barrett wrote:
>>>>> On Apr 7, 2018, at 7:03 AM, Thomas Schatzl <thomas.schatzl at oracle.c
>>>>> om> wrote:
>>>>> 
>>>>> Hi Kim,
>>>>> 
>>>>> On Fri, 2018-04-06 at 19:55 -0400, Kim Barrett wrote:
>>>>>> Please review this fix of a couple of macro definitions that have
>>>>>> "defined" expressions in the expansion.  Such code has undefined
>>>>>> behavior: C++03 16.1/3.  Xcode 9.3 warns about such code.
>>>>>> 
>>>>>> The fix for HEAP_REGION_SET_FORCE_VERIFY is a straight-forward
>>>>>> change to use #ifdef ASSERT to define it appropriately.
>>>>> It would imho be much nicer to remove the define and use the
>>>>> available NOT_DEBUG_RETURN/#ifndef ASSERT macros to accomplish the
>>>>> same thing. (I would also be fine with using
>>>>> NOT_PRODUCT_RETURN/#ifndef
>>>>> PRODUCT).
>>>> That doesn’t accomplish the same thing.  The present code allows one
>>>> to explicitly define the macro on the (make) command line to override
>>>> the default behavior (based on ASSERT).  Are you suggesting removing
>>>> that capability?
>>> I have never anyone seen anyone using extra defines on the command line
>>>  and I am very certain I have never used them myselves, but maybe it
>>> is common to do for others.
>>> I can imagine people using them for initial development, and then
>>> leaving them in "just in case", forgetting them rather quickly. This
>>> and others are certainly not documented anywhere.
>>> 
>>> It is also really easy to forget to remember to always specify the
>>> correct defines, i.e. be sure that the build actually contains this
>>> code too.
>>> 
>>> That particular define has been introduced in 2011 and like many other
>>> defines/paranoia checks/* in G1 code we removed over time this one
>>> seems to duplicate the suggested (PRODUCT_RETURN / NOT_DEBUG_RETURN)
>>> functionality.
>>> 
>>> (Actually tbh, I have heard of HEAP_REGION_SET_FORCE_VERIFY for the
>>> very first time. The heapRegionSet* files are very old parts of G1).
>>> 
>>> Maybe Tony can comment.
>> I'm not going to defend it.  I don't much like this kind of littering
>> of the code base either.  I was trying to fix a build problem on some
>> (not (yet) supported by Oracle) platforms, without making a judgement
>> about the feature.  But I'm happy to remove HEAP_REGION_SET_FORCE_VERIFY
>> instead.
> 
> I agree - this sounds good to me.

Thanks.

>>>> NOT_PRODUCT/#ifndef PRODUCT is the wrong choice, being contrary to to
>>>> the intent of an optimize build to be similar in performance
>>>> characteristics to a product build.
>>> In my understanding there is an additional clause in that sentence:
>>> "[...] to a product build, including some extra verification".
>>> 
>>> There is lots of similar verification that uses PRODUCT_RETURN/#ifndef
>>> PRODUCT that also already changes performance characteristics for an
>>> optimized build.
>>> See the other PRODUCT_RETURN uses (I can find 107 in just the gc
>>> directory) in this and other files.
>> That's not my understanding from discussions with John Masamitsu (who
>> was the only person I knew of to regularly use and care about
>> "optimized" builds).  But it is certainly true there is a lot of code
>> that violates that rule.  I occasionally wonder whether anyone would
>> care or notice if we killed off "optimized" builds; we don't test that
>> mode, with the result that it is often broken.
> 
> We have an RFE open for removing the optimized builds but it's blocked on a compiler change.
> https://bugs.openjdk.java.net/browse/JDK-8183287

It makes sense for the compiler folks to want to use optimized builds.

> Coleen
>> 
>>> So yes, I recommend removing this capability in this instance. This
>>> verification does not seem to be any different than any other.
>>> 
>>> Not sure if this capability is too expensive or not (this is typically
>>> a bit fuzzy), but in the worst case one can use NOT_DEBUG_RETURN.
>> Since it was previously only under ASSERT, I'm leaving it that way.
>> 
>> New webrevs:
>> full: http://cr.openjdk.java.net/~kbarrett/8200550/open.01/
>> incr: http://cr.openjdk.java.net/~kbarrett/8200550/open.01.inc/