RFR (XS) 8047212: fix race between ObjectMonitor alloc and verification code (original) (raw)

Daniel D. Daugherty daniel.daugherty at oracle.com
Wed Oct 21 14:58:27 UTC 2015

On 10/21/15, 1:57 AM, David Holmes wrote:

On 21/10/2015 3:17 PM, Daniel D. Daugherty wrote:

On 10/20/15, 10:27 PM, David Holmes wrote:

On 21/10/2015 1:53 PM, Carsten Varming wrote:

Dear David,

In this case dummytype is the result of a typedef. "typedef int* dummytype; volatile dummytype * dummy" is the same as "typedef int* dummytype; dummytype volatile * dummy". Nevertheless, I always recommend sticking to postfix unary type operators in macros to minimize confusion with substitution. The role of the typedef was tickling something in my memory :) Yes you are right - the typedef solves this problem. Which means that the existing is correct and Dan's new macro is not needed either. The code doesn't build without a new macro. That's why I created it. None of the existing macros work with: static ObjectMonitor * volatile gBlockList; Right, sorry, back to my original comment that all you need is the static version of the existing non-static one.

I tried that route and didn't get very far. Massive compilation failures, but I didn't save those logs...

So the route I took was to create a static ptr volatile version of the existing static one. More below (much more).

I cannot believe we're spending all this time on the name of the macros. But it ain't just about the name.

Right... but I've seen nothing concrete about the code at all. No comments about specific code in the new macros that is wrong. No comments that say "do this" instead of "that".

David -----

Okay so let's take a walk through the woods to see how we got here. Getting vmStructs.cpp happy was such a problem that I did it phases the second time. I saved both the .cpp file and the make logs.

$ ls -ltr hotspot/src/share/vm/runtime/vmStructs.cpp.orig hotspot/src/share/vm/runtime/vmStructs.cpp.add_* -rw-r--r-- 1 ddaugher cr0149 284482 Oct 1 13:41 hotspot/src/share/vm/runtime/vmStructs.cpp.orig -rw-r--r-- 1 ddaugher cr0149 284703 Oct 15 17:13 hotspot/src/share/vm/runtime/vmStructs.cpp.add_VM_STRUCTS_param -rw-r--r-- 1 ddaugher cr0149 284960 Oct 15 17:17 hotspot/src/share/vm/runtime/vmStructs.cpp.add_GENERATE_STATIC_VOLATILE_VM_STRUCT_ENTRY -rw-r--r-- 1 ddaugher cr0149 285213 Oct 15 17:23 hotspot/src/share/vm/runtime/vmStructs.cpp.add_CHECK_STATIC_VOLATILE_VM_STRUCT_ENTRY

$ ls -ltr make.all.linux-x86_64-normal-server-release.log.add_* -rw-r--r-- 1 ddaugher cr0149 2176 Oct 15 17:11 make.all.linux-x86_64-normal-server-release.log.add_VM_STRUCTS_param -rw-r--r-- 1 ddaugher cr0149 1999 Oct 15 17:17 make.all.linux-x86_64-normal-server-release.log.add_GENERATE_STATIC_VOLATILE_VM_STRUCT_ENTRY -rw-r--r-- 1 ddaugher cr0149 13648 Oct 15 17:29 make.all.linux-x86_64-normal-server-release.log.add_CHECK_STATIC_VOLATILE_VM_STRUCT_ENTRY

======= PHASE 1

Phase 1 is to add the new parameter to the VM_STRUCTS macro. While the parameter is named volatile_static_field, we use the existing non-volatile static macros to implement the application of the "volatile_static_field" parameter to the ObjectSynchronizer::gBlockList static field. In other words, this code behaves the same as if I made no changes to vmStructs.cpp.

Here are the diffs:

$ diff hotspot/src/share/vm/runtime/vmStructs.cpp.orig hotspot/src/share/vm/runtime/vmStructs.cpp.add_VM_STRUCTS_param 265a266 > volatile_static_field,
1111c1112 < static_field(ObjectSynchronizer,
gBlockList,
ObjectMonitor*) \

volatile_static_field(ObjectSynchronizer,
gBlockList,
ObjectMonitor*)
2944a2946 GENERATE_STATIC_VM_STRUCT_ENTRY, 3108a3111 CHECK_STATIC_VM_STRUCT_ENTRY, 3229a3233 CHECK_NO_OP, 3232a3237 ENSURE_FIELD_TYPE_PRESENT,

And here is the compiler error that results:

$ cat make.all.linux-x86_64-normal-server-release.log.add_VM_STRUCTS_param Build Started: Thu Oct 15 17:11:24 PDT 2015 Building configuration 'linux-x86_64-normal-server-release' (matching CONF=linux-x86_64-normal-server-release) Building configuration 'linux-x86_64-normal-server-release' (matching CONF=linux-x86_64-normal-server-release) Building target 'all' in configuration 'linux-x86_64-normal-server-release' /work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp:2994:1: error: invalid conversion from 'volatile void*' to 'void*' [-fpermissive] }; ^ /work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp: In static member function 'static void VMStructs::init()': /work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp:1112:48: error: invalid conversion from 'ObjectMonitor* volatile*' to 'ObjectMonitor**' [-fpermissive] volatile_static_field(ObjectSynchronizer,
gBlockList,
ObjectMonitor*)
^ /work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp:2751:28: note: in definition of macro 'CHECK_STATIC_VM_STRUCT_ENTRY' {type* dummy = &typeName::fieldName; } ^ /work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp:3109:3: note: in expansion of macro 'VM_STRUCTS' VM_STRUCTS(CHECK_NONSTATIC_VM_STRUCT_ENTRY, ^ gmake[8]: *** [vmStructs.o] Error 1 gmake[7]: *** [the_vm] Error 2 gmake[6]: *** [product] Error 2 gmake[5]: *** [generic_build2] Error 2 gmake[4]: *** [product] Error 2 gmake[3]: *** [/work/shared/bug_hunt/8047212_for_jdk9_hs_rt/build/linux-x86_64-normal-server-release/hotspot/_hotspot.timestamp] Error 1 gmake[2]: *** [hotspot] Error 1

ERROR: Build failed for target 'all' in configuration 'linux-x86_64-normal-server-release' (exit code 2) No indication of failed target found. Hint: Try searching the build log for '] Error'. Hint: If caused by a warning, try configure --disable-warnings-as-errors.

make381[1]: *** [main] Error 1 make381: *** [all] Error 2

Build Finished: Thu Oct 15 17:11:34 PDT 2015

The above compiler errors are the same as when there are no changes to vmStructs.cpp modulo some changed line numbers because of the placeholder (but equivalent) changes that I made.

Now let's look at the errors:

/work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp:2994:1: error: invalid conversion from 'volatile void*' to 'void*' [-fpermissive] }; ^

The above error does not mention it but it comes from the GENERATE_STATIC_VM_STRUCT_ENTRY macro. The goal of this macro is to create a local variable that is initialized to the target field so that SA can have a copy of the target fields contents. This compiler error happens because the real type definition of ObjectSynchronizer::gBlockList no longer matches the left hand side of the assignment:

invalid conversion from 'volatile void*' to 'void*'

The right hand side now sees "volatile" in the mix while the left hand side still thinks the local variable should be 'void*'.

I tried various things to get the new GENERATE macro to have a properly volatile left hand side, but couldn't get it. Finally I used the dreaded big hammer cast:

Here's the existing static field GENERATE and new static ptr volatile GENERATE macro:

2714 #define GENERATE_STATIC_VM_STRUCT_ENTRY(typeName, fieldName, type)
2715 { QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, &typeName::fieldName },

2719 #define GENERATE_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY(typeName, fieldName, type)
2720 { QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, (void *)&typeName::fieldName },

Other than the new name, the only change is the addition of "(void *)" which make the compiler shutup about the volatile mismatch between the left-hand side and the right-hand side. For this particular use case, the suppression of "volatile" doesn't matter. From SA, we don't care about the "volatile" semantics of gBlockList, we just want to walk the block list.

Now let's look at the second error:

/work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp: In static member function 'static void VMStructs::init()': /work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp:1112:48: error: invalid conversion from 'ObjectMonitor* volatile*' to 'ObjectMonitor**' [-fpermissive] volatile_static_field(ObjectSynchronizer,
gBlockList,
ObjectMonitor*)
^ /work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp:2751:28: note: in definition of macro 'CHECK_STATIC_VM_STRUCT_ENTRY' {type* dummy = &typeName::fieldName; } ^ /work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp:3109:3: note: in expansion of macro 'VM_STRUCTS' VM_STRUCTS(CHECK_NONSTATIC_VM_STRUCT_ENTRY,

The VMStructs::init() function uses the various CHECK_ macros to verify that SA properly understands the expected types of the fields that it knows about.

This compiler error happens because the real type definition of ObjectSynchronizer::gBlockList no longer matches the left hand side of the assignment:

error: invalid conversion from 'ObjectMonitor* volatile*' to 'ObjectMonitor**'

The right hand side now sees "volatile" in the mix while the left hand side still thinks the local variable should be 'ObjectMonitor**'.

Here's the existing static field CHECK and new static ptr volatile CHECK macro:

2748 #define CHECK_STATIC_VM_STRUCT_ENTRY(typeName, fieldName, type)
2749 {type* dummy = &typeName::fieldName; }

2753 #define CHECK_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY(typeName, fieldName, type)
2754 {type volatile * dummy = &typeName::fieldName; }

For this new macro, I was able to inject "volatile" in the right place to make the compiler happy. No casts needed. Since I was only using the new macro for:

static ObjectMonitor * volatile gBlockList;

I picked the new name of "CHECK_STATIC_PTR_VOLATILE_..." because my use case is a ((static ObjectMonitor *) volatile) gBlockList STATIC_PTR_.............VOLATILE

The position of the "volatile" keyword in this case is critical. It must follow "ObjectMonitor *" because it is the pointer that is volatile and not the ObjectMonitor itself that is volatile.

Going from the CHECK_VOLATILE_NONSTATIC_ macro to this new macro didn't work because the "volatile" in that macro is in the wrong place for my use case. Also, you have to remove all the dummy type stuff because we're not trying to craft/type-check the object's type so that we can access the non-static field.

======= PHASE 2

Phase 2 adds the new GENERATE_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY macro that I showed above:

$ diff hotspot/src/share/vm/runtime/vmStructs.cpp.add_VM_STRUCTS_param hotspot/src/share/vm/runtime/vmStructs.cpp.add_GENERATE_STATIC_VOLATILE_VM_STRUCT_ENTRY 2723a2724,2727 > // This macro generates a VMStructEntry line for a static volatile field > #define GENERATE_STATIC_VOLATILE_VM_STRUCT_ENTRY(typeName, fieldName, type)
> { QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, (void *)&typeName::fieldName }, > 2946c2950 < GENERATE_STATIC_VM_STRUCT_ENTRY,

         GENERATE_STATIC_VOLATILE_VM_STRUCT_ENTRY,

And you can see that change removed the first compile error discussed above:

$ cat make.all.linux-x86_64-normal-server-release.log.add_GENERATE_STATIC_VOLATILE_VM_STRUCT_ENTRY Build Started: Thu Oct 15 17:17:15 PDT 2015 Building configuration 'linux-x86_64-normal-server-release' (matching CONF=linux-x86_64-normal-server-release) Building configuration 'linux-x86_64-normal-server-release' (matching CONF=linux-x86_64-normal-server-release) Building target 'all' in configuration 'linux-x86_64-normal-server-release' /work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp: In static member function 'static void VMStructs::init()': /work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp:1112:48: error: invalid conversion from 'ObjectMonitor* volatile*' to 'ObjectMonitor**' [-fpermissive] volatile_static_field(ObjectSynchronizer,
gBlockList,
ObjectMonitor*)
^ /work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp:2755:28: note: in definition of macro 'CHECK_STATIC_VM_STRUCT_ENTRY' {type* dummy = &typeName::fieldName; } ^ /work/shared/bug_hunt/8047212_for_jdk9_hs_rt/hotspot/src/share/vm/runtime/vmStructs.cpp:3113:3: note: in expansion of macro 'VM_STRUCTS' VM_STRUCTS(CHECK_NONSTATIC_VM_STRUCT_ENTRY, ^ gmake[8]: *** [vmStructs.o] Error 1 gmake[7]: *** [the_vm] Error 2 gmake[6]: *** [product] Error 2 gmake[5]: *** [generic_build2] Error 2 gmake[4]: *** [product] Error 2 gmake[3]: *** [/work/shared/bug_hunt/8047212_for_jdk9_hs_rt/build/linux-x86_64-normal-server-release/hotspot/_hotspot.timestamp] Error 1 gmake[2]: *** [hotspot] Error 1

ERROR: Build failed for target 'all' in configuration 'linux-x86_64-normal-server-release' (exit code 2) No indication of failed target found. Hint: Try searching the build log for '] Error'. Hint: If caused by a warning, try configure --disable-warnings-as-errors.

make381[1]: *** [main] Error 1 make381: *** [all] Error 2

Build Finished: Thu Oct 15 17:17:25 PDT 2015

======= PHASE 3

Phase 3 adds the CHECK_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY that I showed above:

$ diff hotspot/src/share/vm/runtime/vmStructs.cpp.add_GENERATE_STATIC_VOLATILE_VM_STRUCT_ENTRY hotspot/src/share/vm/runtime/vmStructs.cpp.add_CHECK_STATIC_VOLATILE_VM_STRUCT_ENTRY 2753c2753 < // This macro checks the type of a VMStructEntry by comparing pointer types

// This macro checks the type of a static VMStructEntry by comparing pointer types 2756a2757,2760 // This macro checks the type of a static volatile VMStructEntry by comparing pointer types #define CHECK_STATIC_VOLATILE_VM_STRUCT_ENTRY(typeName, fieldName, type)
{type volatile * dummy = &typeName::fieldName; }

3115c3119 < CHECK_STATIC_VM_STRUCT_ENTRY,

         CHECK_STATIC_VOLATILE_VM_STRUCT_ENTRY,

The make log in this case is much more boring:

$ head make.all.linux-x86_64-normal-server-release.log.add_CHECK_STATIC_VOLATILE_VM_STRUCT_ENTRY

Build Started: Thu Oct 15 17:23:54 PDT 2015 Building configuration 'linux-x86_64-normal-server-release' (matching CONF=linux-x86_64-normal-server-release) Building configuration 'linux-x86_64-normal-server-release' (matching CONF=linux-x86_64-normal-server-release) Building target 'all' in configuration 'linux-x86_64-normal-server-release' All done.

Running javadoc for images/docs/api/index.html

Compiling 1 files for BUILD_DEMO_APPLET_DrawTest Compiling 1 files for BUILD_DEMO_APPLET_Fractal

We make it past the VM build phase without an error...

So after this very long tale, you now know how I got where I'm at.

Dan

More information about the hotspot-runtime-dev mailing list