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

[Daniel D. Daugherty]

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.


>
> 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
>>>>>>