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

Wed Oct 21 03:53:10 UTC 2015

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.

Carsten

On Tue, Oct 20, 2015 at 8:39 PM, David Holmes <david.holmes at oracle.com>
wrote:

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