RFR (XS) 8047212: fix race between ObjectMonitor alloc and verification code
David Holmes
david.holmes at oracle.com
Thu Oct 22 05:20:05 UTC 2015
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.
Sorry for belabouring this, I really thought it should have been a lot
simpler and clearer to address this. :(
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