RFR: 8159370: Add FlagGuard for easier modification of flags for unit tests

[Kim Barrett]

> On Jun 17, 2016, at 11:31 AM, Erik Helin <erik.helin at oracle.com> wrote:
> 
> On 2016-06-14, Kim Barrett wrote:
>>> On Jun 14, 2016, at 6:58 AM, Erik Helin <erik.helin at oracle.com> wrote:
>>> 
>>> Hi all,
>>> 
>>> this patch adds a small utility class that makes it easier for TEST_VM
>>> unit tests to safely modify flags. The class FlagGuard will store the
>>> value of the flag in its constructor and then restore the flag in its
>>> destructor. This is similar IntFlagSetting, UIntFlagSetting etc, but the
>>> FlagGuard differs in two aspects:
>>> - it does not set a new value
>>> - it is "untyped", you can use FlagGuard for flags with any type
>> 
>> Is there a reason why this is being added to the unit test framework,
>> rather than being a generalization of the existing [U]IntFlagSetting
>> &etc and placed nearby?  That’s where I would expect to find it.
> 
> So my reasoning was that the FlagGuard was pretty slow, since it used
> Flag::find_flag, so I didn't want to encourage anyone to use it in
> production. Me and Per hacked together a new implementation of
> FlagGuard, which uses templates and memcpy instead of Flag::find_flag:
> 
> http://cr.openjdk.java.net/~ehelin/8159370/00/

That's the old URL; the correct URL ends in /01/

> What do you think? I also moved FlagGuard to runtime/globals.hpp and
> added some tests. I don't want to update [U]IntFlagSetting since
> they also take a new value as parameter, FlagGuard just restores the
> value at the end of the scope.

------------------------------------------------------------------------------
For reference, the technique used here is based on C++2003 3.9/2.

The basic idea seems sound, though I have quibbles about details.

I'm fine with this interface and leaving [U]IntFlagSetting alone.

Given that we have a fixed set of types to deal with, I suspect there
is some tag dispatching / sizeof trick that could eliminate the memcpy
and use the actual option type, but it hardly seems worth the trouble.

There are much cleaner solutions with C++11...

------------------------------------------------------------------------------  
src/share/vm/runtime/globals.hpp 
 469   uint8_t _value[SIZE];

Element type should be char or unsigned char.  uint8_t is not
necessarily either of those, though it likely is on any platform we
support.

------------------------------------------------------------------------------   
src/share/vm/runtime/globals.hpp 
 475   void* operator new(size_t size) throw() {
 476     return 0;
 477   }
 478 
 479   void* operator new [](size_t size) throw() {
 480     return 0;
 481   }

Leave off the definitions, further ensuring they can't be used, and
not being misleading about them.

------------------------------------------------------------------------------ 
src/share/vm/runtime/globals.hpp 
 470   const uintptr_t _addr;
 484   FlagGuard(uintptr_t flag_addr) : _addr(flag_addr) {
 493 #define FLAG_GUARD(f) FlagGuard<sizeof(f)> f ## _guard((uintptr_t) &f)

Why the address conversion to uintptr_t?  Why not just use const void*,
and eliminate lots of casts...

------------------------------------------------------------------------------  
src/share/vm/runtime/globals.hpp 
 485     memcpy((void*) _value, (const void*) _addr, SIZE);
 489     memcpy((void*) _addr, (const void*) _value, SIZE);

Unnecessary casts of _value.

------------------------------------------------------------------------------  
test/native/runtime/test_globals.cpp 

This file only contains tests for FlagGuard.  I don't know what
conventions we're adopting for native tests, and what restrictions the
build system might be imposing on us.  It seems to me though that this
might be better called test_FlagGuard, and other native tests of
facilities in globals.hpp would be in other file(s).

------------------------------------------------------------------------------ 
test/native/runtime/test_globals.cpp 

I think each test should also contain a check that the flag is of the
expected type, e.g. at the beginning something like

  ASSERT(Flag::find_flag("AlwaysActAsServerClassMachine")->is_bool());

Otherwise, a change in the type of an option could accidently result
in an untested case here.

------------------------------------------------------------------------------  
test/native/runtime/test_globals.cpp 

The code here is extreme boilerplate.  Consider a macro to reduce
that?

------------------------------------------------------------------------------  
test/native/runtime/test_globals.cpp 
 102   const char* original_value = PerfDataSaveFile;

For consistency, I think that should be ccstr rather than const char*.

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