RFR (XS) 8047212: fix race between ObjectMonitor alloc and verification code
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 serviceability-dev
mailing list