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

David Holmes david.holmes at oracle.com
Wed Oct 21 03:39:04 UTC 2015


On 21/10/2015 1:37 PM, Daniel D. Daugherty wrote:
> On 10/20/15, 9:18 PM, David Holmes wrote:
>> <trimming>
>>
>> On 21/10/2015 12:44 PM, Daniel D. Daugherty wrote:
>>> On 10/20/15, 8:15 PM, David Holmes wrote:
>>>> On 21/10/2015 12:51 AM, Daniel D. Daugherty wrote:
>>>>> On 10/20/15, 1:53 AM, David Holmes wrote:
>>>>>> src/share/vm/runtime/vmStructs.cpp
>>>>>>
>>>>>> Can you not just define volatile_static_field?
>>>>>
>>>>> Yes, I went that way originally and then I changed my mind to
>>>>> avoid colliding with the other format. See below.
>>>>>
>>>>>
>>>>>> Why does the ptr aspect need to come into it? Also "static pointer
>>>>>> volatile field" sounds really odd, it is a static, volatile field
>>>>>> that
>>>>>> happens to be a pointer-type.
>>>>>
>>>>> It's meant to be odd because it follows the actual decl:
>>>>>
>>>>>      static ObjectMonitor * volatile gBlockList;
>>>>>
>>>>> So "static pointer volatile field" is exactly what I have:
>>>>>
>>>>>      static ObjectMonitor * volatile gBlockList;
>>>>>
>>>>>   => (static ObjectMonitor *) volatile gBlockList;
>>>>>
>>>>> i.e. I have a static ObjectMonitor pointer that is volatile
>>>>>
>>>>>
>>>>> Compared to these two forms:
>>>>>
>>>>>      static volatile ObjectMonitor * gBlockList;
>>>>>      static ObjectMonitor volatile * gBlockList;
>>>>>
>>>>>   => static (volatile ObjectMonitor) * gBlockList;
>>>>>   => static (ObjectMonitor volatile) * gBlockList;
>>>>>
>>>>> i.e. I have a static pointer to a volatile ObjectMonitor.
>>>>>
>>>>> Hopefully, this makes my reasons a bit more clear...
>>>>
>>>> Not really :) Yes there is a difference between a "volatile pointer to
>>>> Foo" and "pointer to a volatile Foo", but for the sake of vmstructs we
>>>> don't really seem to need to care about that. The two questions are:
>>>> - is the field/variable static
>>>> - is the field/variable volatile
>>>
>>> I'll have to politely disagree:
>>>
>>> Here's the existing volatile non-static macro:
>>>
>>> 2743 // This macro checks the type of a volatile VMStructEntry by
>>> comparing pointer types
>>> 2744 #define CHECK_VOLATILE_NONSTATIC_VM_STRUCT_ENTRY(typeName,
>>> fieldName, type)        \
>>> 2745  {typedef type dummyvtype; typeName *dummyObj = NULL; volatile
>>> dummyvtype* dummy = &dummyObj->fieldName; }
>>>
>>> And here's the new static pointer volatile macro:
>>>
>>> 2751 // This macro checks the type of a static pointer volatile
>>> VMStructEntry by comparing pointer types,
>>> 2752 // e.g.: "static ObjectMonitor * volatile gBlockList;"
>>> 2753 #define CHECK_STATIC_PTR_VOLATILE_VM_STRUCT_ENTRY(typeName,
>>> fieldName, type)       \
>>> 2754  {type volatile * dummy = &typeName::fieldName; }
>>>
>>> Yes, the variable assignments are different because we have static
>>> versus a non-static situation, but what's more important is where
>>> the "volatile" is positioned.
>>
>> I see your point. But I think the real problem is that there is a bug
>> in the declaration of CHECK_VOLATILE_NONSTATIC_VM_STRUCT_ENTRY that
>> makes it wrong when used with a pointer type. I think this:
>>
>> 2745  {typedef type dummyvtype; typeName *dummyObj = NULL; volatile
>> dummyvtype* dummy = &dummyObj->fieldName; }
>>
>> should really be:
>>
>> 2745  {typedef type dummyvtype; typeName *dummyObj = NULL; dummyvtype
>> volatile * dummy = &dummyObj->fieldName; }
>
> Actually, I believe these two are equivalent:
>
>      volatile dummyvtype* dummy =
>      dummyvtype volatile * dummy =
>
> based on my reading of the URL that I put in the original webrev...
>
> So it's not a bug, it's one variation of an acceptable style.

Not when dummyvtype is itself a pointer type. Consider:

volatile int* *dummy = ...;

Here dummy is a pointer to a pointer to a volatile int.

But in:

int* volatile *dummy = ...;

dummy is a pointer to a volatile pointer to an int

Cheers,
David
-----


>
>>
>> and the static version would follow the same form. dummy is a pointer
>> to a volatile field of type dummyvtype. (I'm unclear why the dummyObj
>> variable is introduced though ??).
>
> 'dummyObj' is used to access the field: &dummyObj->fieldName
>
>
>> I wonder if Kim wants to wade in on this one :)
>
> Dunno.
>
> Dan
>
>
>>
>> Cheers,
>> David
>> -----
>>
>>
>>> In the existing volatile non-static macro, the volatile piece is:
>>>
>>>      volatile dummyvtype* dummy = &dummyObj->fieldName;
>>>
>>> and in the new static pointer volatile macro, the volatile piece is:
>>>
>>>      type volatile * dummy = &typeName::fieldName;
>>>
>>> So the CHECK_VOLATILE_NONSTATIC_XXX macro has the "volatile" before
>>> the data type... and the CHECK_STATIC_PTR_VOLATILE_XXX macro
>>> has the "volatile" after the data type...
>>>
>>> Dan
>>>
>>>
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>> Dan
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> Testing: Aurora Adhoc RT-SVC nightly batch
>>>>>>>           4 inner-complex fastdebug parallel runs for 4+ days and
>>>>>>>             600K iterations without seeing this failure; the
>>>>>>> experiment
>>>>>>>             is still running; final results to be reported at the
>>>>>>> end
>>>>>>>             of the review cycle
>>>>>>>           JPRT -testset hotspot
>>>>>>>
>>>>>>> This fix:
>>>>>>>
>>>>>>> - makes ObjectMonitor::gBlockList volatile
>>>>>>> - uses "OrderAccess::release_store_ptr(&gBlockList, temp)" to
>>>>>>>    make sure the new block updates _happen before_ gBlockList is
>>>>>>>    changed to refer to the new block
>>>>>>> - add SA support for a "static pointer volatile" field like:
>>>>>>>
>>>>>>>      static ObjectMonitor * volatile gBlockList;
>>>>>>>
>>>>>>> See the following link for a nice description of what "volatile"
>>>>>>> means in the different positions on a variable/parameter decl line:
>>>>>>>
>>>>>>> http://www.embedded.com/electronics-blogs/beginner-s-corner/4023801/Introduction-to-the-Volatile-Keyword
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>>>>
>>>>>>> Dan
>>>>>>>


More information about the serviceability-dev mailing list