Reduce risk of the Volatile<T>/VolatilePtr<T,P> structures#124106
Reduce risk of the Volatile<T>/VolatilePtr<T,P> structures#124106davidwrighton wants to merge 6 commits intodotnet:mainfrom
Conversation
- Explicitly delete the copy constructors and equals operators which did not store/load using barriers - Make the value based constructors explicit (since those didn't store with barriers) - Convert assignments of NULL into VolatilePtr to use nullptr so we can go down a consistent path - Adjust the codebase to work with these changes
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR hardens CoreCLR’s Volatile<T> / VolatilePtr<T,P> wrappers by preventing implicit/copy construction and by ensuring assignments between volatile wrappers go through Load/Store paths (and thus respect the intended barrier semantics). It also updates a broad set of call sites to use explicit construction and nullptr to match the new API surface.
Changes:
- Make
Volatile<T>/VolatilePtr<T,P>value-based constructorsexplicit, delete copy/move constructors, and add explicit copy-assignment operators that route throughLoad/Store. - Update many static initializers and assignments across VM/GC/utilcode/JIT to avoid copy-initialization and prefer direct-init +
nullptr. - Introduce
VOLATILE_INIT(val)to allow consistent initialization across configurations whereVOLATILE(T)is eitherT volatileorVolatile<T>.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/vars.cpp | Switch Volatile<LONG> statics to direct-init. |
| src/coreclr/vm/util.cpp | Switch Volatile<double> static to direct-init. |
| src/coreclr/vm/threads.h | Use nullptr for VolatilePtr assignment. |
| src/coreclr/vm/threads.cpp | Use nullptr for several pointer members / fields. |
| src/coreclr/vm/syncclean.cpp | Update VolatilePtr statics to explicit direct-init with nullptr. |
| src/coreclr/vm/runtimecallablewrapper.h | Update macro-local Volatile<LPVOID> to direct-init. |
| src/coreclr/vm/loaderallocator.hpp | Use nullptr for VolatilePtr/pointer reset. |
| src/coreclr/vm/loaderallocator.cpp | Use nullptr for member initialization/reset paths. |
| src/coreclr/vm/jitinterface.cpp | Switch Volatile<int64_t> globals to direct-init. |
| src/coreclr/vm/interoplibinterface_java.cpp | Switch Volatile<bool> global to direct-init. |
| src/coreclr/vm/hosting.cpp | Update debug statics (VolatilePtr/Volatile) to direct-init. |
| src/coreclr/vm/finalizerthread.cpp | Update VolatilePtr static to explicit direct-init. |
| src/coreclr/vm/excep.cpp | Switch Volatile<BOOL> static to direct-init. |
| src/coreclr/vm/eventtrace.cpp | Switch Volatile<LONGLONG> static to direct-init. |
| src/coreclr/vm/eehash.inl | Use nullptr on volatile bucket table reset. |
| src/coreclr/vm/eehash.h | Use nullptr on volatile bucket table initialization. |
| src/coreclr/vm/crst.cpp | Switch Volatile<LONG> global to direct-init. |
| src/coreclr/vm/comcallablewrapper.h | Use nullptr when clearing auxiliary pointers. |
| src/coreclr/vm/comcallablewrapper.cpp | Use nullptr after deleting auxiliary data. |
| src/coreclr/vm/ceemain.cpp | Switch Volatile<BOOL> globals to direct-init. |
| src/coreclr/vm/appdomain.cpp | Use nullptr for member init in initializer list. |
| src/coreclr/utilcode/utsem.cpp | Switch Volatile<BOOL> global to direct-init. |
| src/coreclr/utilcode/util.cpp | Switch Volatile<uint32_t> static to direct-init. |
| src/coreclr/utilcode/stresslog.cpp | Update aggregate init to explicitly construct Volatile<> members. |
| src/coreclr/utilcode/debug.cpp | Switch Volatile<LONG> global to direct-init. |
| src/coreclr/jit/compiler.hpp | Switch local Volatile<> temps to direct-init. |
| src/coreclr/interop/trackerobjectmanager.cpp | Switch Volatile<bool> static to direct-init. |
| src/coreclr/inc/volatile.h | Core change: make constructors explicit, delete copy/move ctors, add assignment ops, add VOLATILE_INIT. |
| src/coreclr/inc/profilepriv.inl | Use nullptr for VolatilePtr reset. |
| src/coreclr/inc/daccess.h | Update VOLATILE_SVAL_IMPL_INIT to use direct-init. |
| src/coreclr/gc/objecthandle.cpp | Use VOLATILE_INIT for static initialization. |
| src/coreclr/gc/gcscan.cpp | Use VOLATILE_INIT for static initialization. |
| src/coreclr/gc/gcpriv.h | Add gc_mechanisms constructors to support new volatile wrapper restrictions. |
| src/coreclr/gc/gceventstatus.cpp | Update array initialization to explicitly construct Volatile<> elements. |
| src/coreclr/gc/gccommon.cpp | Use VOLATILE_INIT for global initialization. |
| src/coreclr/gc/gc.cpp | Update many VOLATILE(...) statics to direct-init and adjust gc_mechanisms copy path. |
| src/coreclr/gc/env/volatile.h | Mirror volatile-wrapper hardening in GC env header and add VOLATILE_INIT. |
| // | ||
| inline VolatilePtr<T, P>& operator=(P val) {this->Store(val); return *this;} | ||
| inline VolatilePtr<T, P>& operator=(const VolatilePtr<T, P>& val) {this->Store(val.Load()); return *this;} | ||
| inline VolatilePtr<T, P>& operator=(VolatilePtr<T, P>&& val) = delete; | ||
| // nullptr is assigned via nullptr_t | ||
| inline VolatilePtr<T, P>& operator=(std::nullptr_t val) {this->Store((P)val); return *this;} |
There was a problem hiding this comment.
std::nullptr_t is used here but this header doesn’t include <cstddef>, so compilation can fail depending on include order. Add #include <cstddef> (or use decltype(nullptr) instead of std::nullptr_t) to make the header self-contained.
| // | |
| inline VolatilePtr<T, P>& operator=(P val) {this->Store(val); return *this;} | |
| inline VolatilePtr<T, P>& operator=(const VolatilePtr<T, P>& val) {this->Store(val.Load()); return *this;} | |
| inline VolatilePtr<T, P>& operator=(VolatilePtr<T, P>&& val) = delete; | |
| // nullptr is assigned via nullptr_t | |
| inline VolatilePtr<T, P>& operator=(std::nullptr_t val) {this->Store((P)val); return *this;} | |
| // Assignment from P | |
| // | |
| inline VolatilePtr<T, P>& operator=(P val) {this->Store(val); return *this;} | |
| inline VolatilePtr<T, P>& operator=(const VolatilePtr<T, P>& val) {this->Store(val.Load()); return *this;} | |
| inline VolatilePtr<T, P>& operator=(VolatilePtr<T, P>&& val) = delete; | |
| // nullptr is assigned via nullptr_t | |
| inline VolatilePtr<T, P>& operator=(decltype(nullptr) val) {this->Store((P)val); return *this;} |
- Minor changes to profiler/Module behavior. Technically this is a fix for the Reflection.Emit scenario, but its unlikely it will impact well written code - The usage in the GC is much more interesting. The rewritten logic here is equivalent to what we had before, but it may be a bug that alloc_context_count is not actually maintained in an accurate fashion.
…n/runtime into AdjustVolatileUsage
These changes are a less suitable for backport version of the work in #124096 These changes reduce the risky surface area of Volatile to a small set of explicit constructors and the GetPointer and operator& apis.
Reasons why still DRAFT
[ ] Investigate problems with getting rid of the operator& apis.
[ ] Investigate problems with getting rid of the GetPointer api