RFR: 8200550: Xcode 9.3 produce warning on heapRegionSet.hpp (original) (raw)

Kim Barrett kim.barrett at oracle.com
Mon Apr 9 06:55:49 UTC 2018

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 HEAPREGIONSETFORCEVERIFY 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 NOTDEBUGRETURN/#ifndef ASSERT macros to accomplish the same thing. (I would also be fine with using NOTPRODUCTRETURN/#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 (PRODUCTRETURN / NOTDEBUGRETURN) functionality. (Actually tbh, I have heard of HEAPREGIONSETFORCEVERIFY 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.

NOTPRODUCT/#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 PRODUCTRETURN/#ifndef PRODUCT that also already changes performance characteristics for an optimized build. See the other PRODUCTRETURN 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.

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

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