RFR: 8200550: Xcode 9.3 produce warning on heapRegionSet.hpp
kim.barrett at oracle.com
Mon Apr 9 20:07:46 UTC 2018
> 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:
>>> 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
>>>> 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)
>>> (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
> I agree - this sounds good to me.
>>>> 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.
It makes sense for the compiler folks to want to use optimized builds.
>>> 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/
More information about the hotspot-dev