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

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: 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 volatilestaticfield?

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 CHECKVOLATILENONSTATICVMSTRUCTENTRY(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 CHECKSTATICPTRVOLATILEVMSTRUCTENTRY(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 CHECKVOLATILENONSTATICVMSTRUCTENTRY 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 CHECKVOLATILENONSTATICXXX macro has the "volatile" before the data type... and the CHECKSTATICPTRVOLATILEXXX 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::releasestoreptr(&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