RFR (XS) 8047212: fix race between ObjectMonitor alloc and verification code
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Oct 22 13:47:17 UTC 2015
On 10/21/15, 11:20 PM, David Holmes wrote:
> On 22/10/2015 9:05 AM, David Holmes wrote:
>> Dan,
>>
>> Sorry I can't wade through this in depth now. A key point is that the
>> use of the typedef removes the problem of whether you use say 'volatile
>> Type' or 'Type volatile' when Type is a pointer type.
>
> Except it doesn't seem to in these macros as you describe below and as
> I also found. Okay I admit defeat.
Not defeat. Due diligence! This does make me wonder if we should do
a 'volatile' keyword audit to make sure that 'volatile' is placed
where it needs to be to achieve the intended effect...
> Sorry for belabouring this, I really thought it should have been a lot
> simpler and clearer to address this. :(
No worries. The stress tests are still running and I want a minimum
of a three day run before I push this fix.
Thanks for the thorough reviews (as always)!
Dan
P.S.
I hope you realize that the solution to this long standing
bug did not reveal itself until just before you returned
from vacation. The bug _knew_ you had to a reviewer! :-)
>
> Thanks,
> David
>
>
>> Will try to go into this further when I am back in the office this
>> afternoon.
>>
>> David
>>
>> On 22/10/2015 12:58 AM, Daniel D. Daugherty wrote:
>>> 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