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

[Erik Helin]

On 2016-06-17, Kim Barrett wrote:
> > 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/

Thanks for correcting the URL. New patches are located at:
- incremental: http://cr.openjdk.java.net/~ehelin/8159370/01-02/
- full: http://cr.openjdk.java.net/~ehelin/8159370/02/

(tested in JRPT)

Please see my comments inline.

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

Thanks!

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

Ok, I changed _value to be unsigned char.

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

Agree, fixed.

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

Agree, changed to void*.

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

Fixed.

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

I would like to start by using one test_ file per source file until we
have more experience with the unit tests. If we then find that it is
better to use multiple test_ files for one source files, then I'll be
happy to move the FlagGuard tests into its own file.

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

Agree, fixed.

> ------------------------------------------------------------------------------  
> test/native/runtime/test_globals.cpp 
> 
> The code here is extreme boilerplate.  Consider a macro to reduce
> that?

Yep, agree. What do you think of the macro?

> ------------------------------------------------------------------------------  
> test/native/runtime/test_globals.cpp 
>  102   const char* original_value = PerfDataSaveFile;
> 
> For consistency, I think that should be ccstr rather than const char*.

Agree, fixed.

Thanks,
Erik