RFR (XS) 8047212: fix race between ObjectMonitor alloc and verification code
David Holmes
david.holmes at oracle.com
Wed Oct 21 07:57:47 UTC 2015
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 cannot believe we're spending all this time on
> the name of the macros.
But it ain't just about the name.
David
-----
> Dan
>
>
>>
>> Thanks,
>> David
>> -----
>>
>>> Carsten
>>>
>>> On Tue, Oct 20, 2015 at 8:39 PM, David Holmes <david.holmes at oracle.com
>>> <mailto: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
>>>
>>>
More information about the hotspot-runtime-dev
mailing list