RFR (XS) 8047212: fix race between ObjectMonitor alloc and verification code
David Holmes
david.holmes at oracle.com
Wed Oct 21 23:05:27 UTC 2015
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.
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 serviceability-dev
mailing list