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 <[email protected]> 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 >>>>>>>> >>>>>>>>
