RFR (XS) 8047212: fix race between ObjectMonitor alloc and verification code
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Oct 21 05:17:24 UTC 2015
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;
I cannot believe we're spending all this time on
the name of the macros.
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