RFR(S): 8195691: AIX build broken after 8194312

[Doerr, Martin]

Hi Kim,

thanks a lot for the detailed explanations.

1. Unfortunately, the friend proposal didn't help.
2. The const proposal didn't work, either:
test/hotspot/gtest/gc/shared/test_oopStorage.cpp", line 1203.13: 1540-0274 (S) The name lookup for "NULL_BLOCK" did not find a declaration.

You're right, that OopBlock* should be used. I have changed that and moved the macro below the typedef.

I have also added some comments which you have suggested.

I have uploaded a new webrev here:
http://cr.openjdk.java.net/~mdoerr/8195691_fix_AIX_build/webrev.01/

Please review and push it when it's ok.

Thanks and best regards,
Martin


-----Original Message-----
From: Kim Barrett [mailto:kim.barrett at oracle.com] 
Sent: Donnerstag, 18. Januar 2018 20:29
To: Doerr, Martin <martin.doerr at sap.com>
Cc: hotspot-runtime-dev at openjdk.java.net; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
Subject: Re: RFR(S): 8195691: AIX build broken after 8194312

> On Jan 18, 2018, at 11:51 AM, Doerr, Martin <martin.doerr at sap.com> wrote:
> 
> Hi Kim,
>  
> 8194312 introduced 2 build problems for xlC (version 12.01.0000.0019):
> 1. test/hotspot/gtest/gc/shared/test_oopStorage.cpp", line 1196.25: 1540-0300 (S) The "private" member "class OopStorage::BlockList" cannot be accessed.
> 2. test/fmw/gtest/include/gtest/gtest.h", line 1448.7: 1540-0207 (S) No common type found for operands with type "long" and "OopStorage::Block *".
>  
> I have found a way to get it working again. Please review:
> http://cr.openjdk.java.net/~mdoerr/8195691_fix_AIX_build/webrev.00/
>  
> For 1., I have made the classes public for AIX only.
> For 2., I have introduced NULL_BLOCK to help the compiler to derive the type.
>  
> Best regards,
> Martin

------------------------------------------------------------------------------  
test/hotspot/gtest/gc/shared/test_oopStorage.cpp
  50 // Using EXPECT_EQ can't use NULL directly. Otherwise AIX build breaks.
  51 #define NULL_BLOCK ((OopStorage::Block*)NULL)

I think this won't work without the other change to make Block public,
which is AIX-conditional, while this change is not.

This should instead refer to the later OopBlock typedef.  Hence I
would prefer the macro be moved to after that typedef.

I think this change probably shouldn't be AIX-specific; while other
compilers seem to be able to cope (e.g. by using some special type for
the value of NULL), that isn't required.  But rather than a macro, I'd
prefer a constant, e.g.

  const OopBlock* const NULL_BLOCK = NULL;

------------------------------------------------------------------------------
 176   // xlC on AIX can't compile test_oopStorage.cpp when the following classes are private.
 177 NOT_AIX( private: )

In private mail I provided a different solution involving friend
declarations that I think is better, assuming it works.  Also
discussed what I think is going on here.  Regardless of how we work
around it, I'd like to leave a comment referring to C++ DR45 here, to
help later readers understand and perhaps someday remove the
workaround.

Here's the theory I previously sent in private email:

-----

OopStorage has some private nested classes: BlockEntry, BlockList, and
Block.

In order to unit test OopStorage, the gtest-based unit tests need
access to those nested class types.

In order to provide the unit tests with that access, OopStorage
declares, but does not define, the public class
OopStorage::TestAccess.  That class is defined as part of the
unit-test.  It "exports" the needed private types by providing public
typedefs for them.

C++03 11.8/1 says "The members of a nested class have no special
access to members of an enclosing class, ...".  That means that
OopStorage::TestAccess shouldn't have access to those private nested
classes, and so can't perform the desired export.

However,
http://www.open-std.org/JTC1/SC22/WG21/docs/cwg_defects.html#45
changed things to give nested classes the same access rights of
similarly placed members.  (The detailed edits for C++11 are a bit
more complicated, because there were other nearby changes.)

All the other compilers we're using have treated DR45 as a bug in
C++03 that can and should be fixed without requiring any special
flags; so one doesn't need to specify -std=c++11 for gcc, for example,
to get this change.

AIX seems to be strangely not complaining about some cases (at least,
you haven't reported failures for the BlockList& cases), and there are
other cases that seem like they ought to be blocked by the old rule
that aren't being complained about either.  So it appears to me that
the AIX compiler is not completely implementing the old rule, nor is
it completely implementing the DR45 changes.

------------------------------------------------------------------------------