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

[Thomas Schatzl]

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.

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

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.

> > > The fix for TAIL_CALL is more interesting.  The code previously
> > > involved "defined(WINDOWS)".  It turns out we don't define
> > > WINDOWS!
> > > The correct macro to test is _WINDOWS.
> > 
> > Please update the CR subject because this is not about
> > heapRegionSet.hpp alone any more before pushing.
> 
> Done.
> 

Thanks,
  Thomas