RFR(S): 8159817: Convert FreeRegionList_test to GTest

[Kim Barrett]

> On Aug 10, 2016, at 1:11 PM, Kirill Zhaldybin <kirill.zhaldybin at oracle.com> wrote:
> 
> Kim,
> 
> Thank you for reviewing the fix.
> 
> Here are new WebRev:
> WebRev: http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8159817/webrev.02/
> 
> Changes:
> 1. Moved code which creates child function to a separate macro, slightly improved error output and made 2 small helper macros.
> 2. Deleted (char*) which actually was not needed.
> 
> Could you please also read comments inline?
> 
> Thank you.
> 
> Regards, Kirill
> 
> On 10.08.2016 00:13, Kim Barrett wrote:
>>> On Aug 5, 2016, at 12:01 PM, Kirill Zhaldybin <kirill.zhaldybin at oracle.com> wrote:
>>> 
>>> Dear all,
>>> 
>>> Could you please review this fix for JDK-8159817?
>>> 
>>> Since this test - freeRegionList - could only be run with G1 GC I added a new macro
>>> TEST_OTHER_VM_WITH_FLAGS which runs a test in a new VM which is started with specified command line options.
>>> 
>>> WebRev: http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8159817/webrev.00/
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8159817
>>> 
>>> Thank you.
>>> 
>>> Regards, Kirill
>> I'm uncomfortable with the new TEST_OTHER_VM_WITH_FLAGS.  I think this
>> may run into problems in conjunction with options passed in to the
>> main gtest execution.  Jesper and I had a bit of discussion about that
>> about a month ago; I thought he was going to file an RFE, but couldn't
>> find one, only a bit of the email exchange.
>> 
>> As an example, if the main test execution is invoked with
>> -XX:+UseXXXGC where XXX is not G1, then the TEST_OTHER_VM_WITH_FLAGS
>> for the new test will lead to problems because of multiple GCs being
>> requested for the child VM.  Just in general I'm not sure we have
>> facilities for (or even a design for) turning tests on or off based on
>> options, and I suspect we need that.
> TEST_OTHER_VM_WITH_FLAGS rewrites _JAVA_OPTIONS without conjunction with existing ones so we cannot get two GCs specified.
> As far as I understand from this - https://bugs.openjdk.java.net/browse/JDK-8158106 - gtests are not supposed to have any external flags.

It is the claim that gtests are not supposed to have any external
flags that Jesper and I were questioning.  That might even be right,
but may need discussion.

I'd missed that the options are being completely replaced.  Sorry for
the confusion.

However, in the environment restoration, if _JAVA_OPTIONS was not
present at all, it gets added with an empty value on exit.  That
doesn't seem right.  I think the safe_old_flags part is a mistake, and
the exit code should be using either putenv (to restore a non-null
old_flags) or unsetenv (to restore a non-present value).

However, this may all be moot for now, since I'm not sure we need
TEST_OTHER_VM_WITH_FLAGS for this test.  See below for discussion of
parameterizing verify_list().

------------------------------------------------------------------------------ 
test/native/unittest.hpp
  56 #define str(s) #s

"str" is way too likely to result in unexpected macro captures.
Suggest TEST_STRINGIFY, assuming this is still needed, e.g. if
TEST_OTHER_VM_WITH_FLAGS is needed.

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

>> Also, I think the use of a child VM here is related to using
>> FreeRegionList::verify_list() in the test, without gtestifying it,
>> e.g. converting assert/guarantee to gtest assertions.  I don't think
>> this test actually needs (or even wants) an initialized VM, but
>> running it in the main VM and having verify_list() report failure
>> would kill the main test VM without allowing any other tests to run.
> I did not do deep investigation but G1RegionToSpaceMapper::create_mapper fails if the test runs with SerialGC

If the gtestrunner is not supposed to receive any VM options, how
would UseSerialGC get used?  Maybe if in some build configuration
where G1 isn't present and only Serial is available?  But that
suggests some GC-related gtests (like this one) should be
conditionalized with #ifdef INCLUDE_ALL_GCS (the newly converted test
has that), and perhaps others should start with something like "if
(!UseG1GC) return;".  After all, the old test being replaced was
protected in such a fashion.

>> Of course, verify_list() is also used in normal code, so we might need
>> two versions of it.  But having two versions is messy unless there is
>> a single verify_list that is somehow parameterized for different
>> reporting mechanisms.
> I did not find a good solution with verify_list().
> First I tried to duplicate its functionality outside of FreeRegionList but since the verification process depends on private FreeRegionList variables it did not look like a possible solution.
> Having parametrized version of verify_list() could require dependencies on gtest in FreeRegionList which looked  not very good too.
> Since the test runs in separate VM and does not impact others I thought that we could leave verify_list() as is so far.

What I meant by parameterizing verify_list() was to give it some sort
of callable argument for reporting failures (a plain function pointer
or an object of a class with a virtual function for performing the
reporting).  So something like

class TestFailureReporter {
public:
  void report(const char* failed_expr,
              const char* filename,
              int line_number,
              const char* fmt,
              ...) const ATTRIBUTE_PRINTF(5, 6);

  virtual void vreport(const char* failed_expr,
                       const char* filename,
                       int line_number,
                       const char* fmt,
                       va_list ap) const ATTRIBUTE_PRINTF(5, 0) = 0;

  virtual ~TestFailureReporter();
};		      

There are two implementations of TestFailureReporter.  One uses
report_vm_error, the other uses gtest's ADD_FAILURE_AT and is provided
by our unit test framework.

Also add a macro something like (note, VERIFY is probably not the best
name for this, as it might be too generic)

  #define VERIFY(expr, reporter, ...) \
    ((expr) || ((reporter)(XSTR(expr), __FILE__, __LINE__, __VA_ARGS__), false)

That's what I meant by missing infrastructure.

Once we have that, we can change the signature of verify_list() to

  void verify_list(const TestReportFailure& reporter) const;

and replace the guarantee statements in verify_list with invocations
of the above VERIFY macro.

>> [pre-existing] There are other moderately complicated and frequently
>> duplicated chunks in this file, with more occurrences added by
>> TEST_OTHER_VM_WITH_FLAGS.  For example, I see many occurrences of
>> 
>>   test_ ## category ## _ ## name ## _()
> Well, we could change it to some macro but it will be still something like TEST_FUNCTION(category, name). Do you think it worth it?

I dislike repeated relatively complex "expressions" like this, but it
might not be worth the macro namespace pollution to do something about
that repetition in this case.