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

David Holmes david.holmes at oracle.com
Wed Oct 21 23:05:27 UTC 2015

Dan,

Sorry I can't wade through this in depth now. A key point is that the use of the typedef removes the problem of whether you use say 'volatile Type' or 'Type volatile' when Type is a pointer type.

Will try to go into this further when I am back in the office this afternoon.

David

On 22/10/2015 12:58 AM, Daniel D. Daugherty wrote:

On 10/21/15, 1:57 AM, David Holmes wrote:

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 tried that route and didn't get very far. Massive compilation failures, but I didn't save those logs... So the route I took was to create a static ptr volatile version of the existing static one. More below (much more). I cannot believe we're spending all this time on the name of the macros. But it ain't just about the name. Right... but I've seen nothing concrete about the code at all. No comments about specific code in the new macros that is wrong. No comments that say "do this" instead of "that". David ----- Okay so let's take a walk through the woods to see how we got here. Getting vmStructs.cpp happy was such a problem that I did it phases the second time. I saved both the .cpp file and the make logs. $ ls -ltr hotspot/src/share/vm/runtime/vmStructs.cpp.orig hotspot/src/share/vm/runtime/vmStructs.cpp.add* -rw-r--r-- 1 ddaugher cr0149 284482 Oct 1 13:41 hotspot/src/share/vm/runtime/vmStructs.cpp.orig -rw-r--r-- 1 ddaugher cr0149 284703 Oct 15 17:13 hotspot/src/share/vm/runtime/vmStructs.cpp.addVMSTRUCTSparam -rw-r--r-- 1 ddaugher cr0149 284960 Oct 15 17:17 hotspot/src/share/vm/runtime/vmStructs.cpp.addGENERATESTATICVOLATILEVMSTRUCTENTRY -rw-r--r-- 1 ddaugher cr0149 285213 Oct 15 17:23 hotspot/src/share/vm/runtime/vmStructs.cpp.addCHECKSTATICVOLATILEVMSTRUCTENTRY $ ls -ltr make.all.linux-x8664-normal-server-release.log.add* -rw-r--r-- 1 ddaugher cr0149 2176 Oct 15 17:11 make.all.linux-x8664-normal-server-release.log.addVMSTRUCTSparam -rw-r--r-- 1 ddaugher cr0149 1999 Oct 15 17:17 make.all.linux-x8664-normal-server-release.log.addGENERATESTATICVOLATILEVMSTRUCTENTRY -rw-r--r-- 1 ddaugher cr0149 13648 Oct 15 17:29 make.all.linux-x8664-normal-server-release.log.addCHECKSTATICVOLATILEVMSTRUCTENTRY ======= PHASE 1 ======= Phase 1 is to add the new parameter to the VMSTRUCTS macro. While the parameter is named volatilestaticfield, we use the existing non-volatile static macros to implement the application of the "volatilestaticfield" parameter to the ObjectSynchronizer::gBlockList static field. In other words, this code behaves the same as if I made no changes to vmStructs.cpp. Here are the diffs: $ diff hotspot/src/share/vm/runtime/vmStructs.cpp.orig hotspot/src/share/vm/runtime/vmStructs.cpp.addVMSTRUCTSparam 265a266 _> volatilestaticfield, _ 1111c1112 < staticfield(ObjectSynchronizer, gBlockList,_ _ObjectMonitor*) _ _---_ _> volatilestaticfield(ObjectSynchronizer, gBlockList, _ObjectMonitor*) _ 2944a2946 > GENERATESTATICVMSTRUCTENTRY, 3108a3111 > CHECKSTATICVMSTRUCTENTRY, 3229a3233 > CHECKNOOP, 3232a3237 > ENSUREFIELDTYPEPRESENT, And here is the compiler error that results: $ cat make.all.linux-x8664-normal-server-release.log.addVMSTRUCTSparam Build Started: Thu Oct 15 17:11:24 PDT 2015 Building configuration 'linux-x8664-normal-server-release' (matching CONF=linux-x8664-normal-server-release) Building configuration 'linux-x8664-normal-server-release' (matching CONF=linux-x8664-normal-server-release) Building target 'all' in configuration 'linux-x8664-normal-server-release' /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp:2994:1: error: invalid conversion from 'volatile void*' to 'void*' [-fpermissive] }; ^ /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp: In static member function 'static void VMStructs::init()': /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp:1112:48: error: invalid conversion from 'ObjectMonitor* volatile*' to 'ObjectMonitor**' [-fpermissive] volatilestaticfield(ObjectSynchronizer, gBlockList, _ObjectMonitor*) _ ^ /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp:2751:28: note: in definition of macro 'CHECKSTATICVMSTRUCTENTRY' {type* dummy = &typeName::fieldName; } ^ /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp:3109:3: note: in expansion of macro 'VMSTRUCTS' VMSTRUCTS(CHECKNONSTATICVMSTRUCTENTRY, ^ gmake[8]: *** [vmStructs.o] Error 1 gmake[7]: *** [thevm] Error 2 gmake[6]: *** [product] Error 2 gmake[5]: *** [genericbuild2] Error 2 gmake[4]: *** [product] Error 2 gmake[3]: *** [/work/shared/bughunt/8047212forjdk9hsrt/build/linux-x8664-normal-server-release/hotspot/hotspot.timestamp] Error 1 gmake[2]: *** [hotspot] Error 1 ERROR: Build failed for target 'all' in configuration 'linux-x8664-normal-server-release' (exit code 2) No indication of failed target found. Hint: Try searching the build log for '] Error'. Hint: If caused by a warning, try configure --disable-warnings-as-errors. make381[1]: *** [main] Error 1 make381: *** [all] Error 2 Build Finished: Thu Oct 15 17:11:34 PDT 2015 The above compiler errors are the same as when there are no changes to vmStructs.cpp modulo some changed line numbers because of the placeholder (but equivalent) changes that I made. Now let's look at the errors: /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp:2994:1: error: invalid conversion from 'volatile void*' to 'void*' [-fpermissive] }; ^ The above error does not mention it but it comes from the GENERATESTATICVMSTRUCTENTRY macro. The goal of this macro is to create a local variable that is initialized to the target field so that SA can have a copy of the target fields contents. This compiler error happens because the real type definition of ObjectSynchronizer::gBlockList no longer matches the left hand side of the assignment: invalid conversion from 'volatile void*' to 'void*' The right hand side now sees "volatile" in the mix while the left hand side still thinks the local variable should be 'void*'. I tried various things to get the new GENERATE macro to have a properly volatile left hand side, but couldn't get it. Finally I used the dreaded big hammer cast: Here's the existing static field GENERATE and new static ptr volatile GENERATE macro: 2714 #define GENERATESTATICVMSTRUCTENTRY(typeName, fieldName, _type) _ 2715 { QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, &typeName::fieldName }, 2719 #define GENERATESTATICPTRVOLATILEVMSTRUCTENTRY(typeName, _fieldName, type) _ 2720 { QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, (void *)&typeName::fieldName }, Other than the new name, the only change is the addition of "(void *)" which make the compiler shutup about the volatile mismatch between the left-hand side and the right-hand side. For this particular use case, the suppression of "volatile" doesn't matter. From SA, we don't care about the "volatile" semantics of gBlockList, we just want to walk the block list. Now let's look at the second error: /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp: In static member function 'static void VMStructs::init()': /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp:1112:48: error: invalid conversion from 'ObjectMonitor* volatile*' to 'ObjectMonitor**' [-fpermissive] volatilestaticfield(ObjectSynchronizer, gBlockList, _ObjectMonitor*) _ ^ /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp:2751:28: note: in definition of macro 'CHECKSTATICVMSTRUCTENTRY' {type* dummy = &typeName::fieldName; } ^ /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp:3109:3: note: in expansion of macro 'VMSTRUCTS' VMSTRUCTS(CHECKNONSTATICVMSTRUCTENTRY, The VMStructs::init() function uses the various CHECK macros to verify that SA properly understands the expected types of the fields that it knows about. This compiler error happens because the real type definition of ObjectSynchronizer::gBlockList no longer matches the left hand side of the assignment: error: invalid conversion from 'ObjectMonitor* volatile*' to 'ObjectMonitor**' The right hand side now sees "volatile" in the mix while the left hand side still thinks the local variable should be 'ObjectMonitor**'. Here's the existing static field CHECK and new static ptr volatile CHECK macro: 2748 #define CHECKSTATICVMSTRUCTENTRY(typeName, fieldName, _type) _ 2749 {type* dummy = &typeName::fieldName; } 2753 #define CHECKSTATICPTRVOLATILEVMSTRUCTENTRY(typeName, _fieldName, type) _ 2754 {type volatile * dummy = &typeName::fieldName; } For this new macro, I was able to inject "volatile" in the right place to make the compiler happy. No casts needed. Since I was only using the new macro for: static ObjectMonitor * volatile gBlockList; I picked the new name of "CHECKSTATICPTRVOLATILE..." because my use case is a ((static ObjectMonitor *) volatile) gBlockList STATICPTR.............VOLATILE The position of the "volatile" keyword in this case is critical. It must follow "ObjectMonitor *" because it is the pointer that is volatile and not the ObjectMonitor itself that is volatile. Going from the CHECKVOLATILENONSTATIC macro to this new macro didn't work because the "volatile" in that macro is in the wrong place for my use case. Also, you have to remove all the dummy type stuff because we're not trying to craft/type-check the object's type so that we can access the non-static field. ======= PHASE 2 ======= Phase 2 adds the new GENERATESTATICPTRVOLATILEVMSTRUCTENTRY macro that I showed above: $ diff hotspot/src/share/vm/runtime/vmStructs.cpp.addVMSTRUCTSparam hotspot/src/share/vm/runtime/vmStructs.cpp.addGENERATESTATICVOLATILEVMSTRUCTENTRY 2723a2724,2727 > // This macro generates a VMStructEntry line for a static volatile field > #define GENERATESTATICVOLATILEVMSTRUCTENTRY(typeName, fieldName, _type) _ > { QUOTE(typeName), QUOTE(fieldName), QUOTE(type), 1, 0, (void *)&typeName::fieldName }, > 2946c2950 < GENERATESTATICVMSTRUCTENTRY,_ _---_ _> GENERATESTATICVOLATILEVMSTRUCTENTRY, And you can see that change removed the first compile error discussed above: $ cat make.all.linux-x8664-normal-server-release.log.addGENERATESTATICVOLATILEVMSTRUCTENTRY Build Started: Thu Oct 15 17:17:15 PDT 2015 Building configuration 'linux-x8664-normal-server-release' (matching CONF=linux-x8664-normal-server-release) Building configuration 'linux-x8664-normal-server-release' (matching CONF=linux-x8664-normal-server-release) Building target 'all' in configuration 'linux-x8664-normal-server-release' /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp: In static member function 'static void VMStructs::init()': /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp:1112:48: error: invalid conversion from 'ObjectMonitor* volatile*' to 'ObjectMonitor**' [-fpermissive] volatilestaticfield(ObjectSynchronizer, gBlockList, _ObjectMonitor*) _ ^ /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp:2755:28: note: in definition of macro 'CHECKSTATICVMSTRUCTENTRY' {type* dummy = &typeName::fieldName; } ^ /work/shared/bughunt/8047212forjdk9hsrt/hotspot/src/share/vm/runtime/vmStructs.cpp:3113:3: note: in expansion of macro 'VMSTRUCTS' VMSTRUCTS(CHECKNONSTATICVMSTRUCTENTRY, ^ gmake[8]: *** [vmStructs.o] Error 1 gmake[7]: *** [thevm] Error 2 gmake[6]: *** [product] Error 2 gmake[5]: *** [genericbuild2] Error 2 gmake[4]: *** [product] Error 2 gmake[3]: *** [/work/shared/bughunt/8047212forjdk9hsrt/build/linux-x8664-normal-server-release/hotspot/hotspot.timestamp] Error 1 gmake[2]: *** [hotspot] Error 1 ERROR: Build failed for target 'all' in configuration 'linux-x8664-normal-server-release' (exit code 2) No indication of failed target found. Hint: Try searching the build log for '] Error'. Hint: If caused by a warning, try configure --disable-warnings-as-errors. make381[1]: *** [main] Error 1 make381: *** [all] Error 2 Build Finished: Thu Oct 15 17:17:25 PDT 2015 ======= PHASE 3 ======= Phase 3 adds the CHECKSTATICPTRVOLATILEVMSTRUCTENTRY that I showed above: $ diff hotspot/src/share/vm/runtime/vmStructs.cpp.addGENERATESTATICVOLATILEVMSTRUCTENTRY hotspot/src/share/vm/runtime/vmStructs.cpp.addCHECKSTATICVOLATILEVMSTRUCTENTRY 2753c2753 < // This macro checks the type of a VMStructEntry by comparing pointer_ _types_ _---_ _> // This macro checks the type of a static VMStructEntry by comparing pointer types 2756a2757,2760 > // This macro checks the type of a static volatile VMStructEntry by comparing pointer types > #define CHECKSTATICVOLATILEVMSTRUCTENTRY(typeName, fieldName, _type) _ > {type volatile * dummy = &typeName::fieldName; } > 3115c3119 < CHECKSTATICVMSTRUCTENTRY,_ _---_ _> CHECKSTATICVOLATILEVMSTRUCTENTRY, The make log in this case is much more boring: $ head make.all.linux-x8664-normal-server-release.log.addCHECKSTATICVOLATILEVMSTRUCTENTRY Build Started: Thu Oct 15 17:23:54 PDT 2015 Building configuration 'linux-x8664-normal-server-release' (matching CONF=linux-x8664-normal-server-release) Building configuration 'linux-x8664-normal-server-release' (matching CONF=linux-x8664-normal-server-release) Building target 'all' in configuration 'linux-x8664-normal-server-release' All done. # Running javadoc for images/docs/api/index.html Compiling 1 files for BUILDDEMOAPPLETDrawTest Compiling 1 files for BUILDDEMOAPPLETFractal We make it past the VM build phase without an error... So after this very long tale, you now know how I got where I'm at. Dan

More information about the hotspot-runtime-dev mailing list