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

[David Holmes]

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

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 ??).

I wonder if Kim wants to wade in on this one :)

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