RFR (XS) 8047212: fix race between ObjectMonitor alloc and verification code
David Holmes
david.holmes at oracle.com
Wed Oct 21 04:27:14 UTC 2015
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.
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