Skip to content

Reduce risk of the Volatile<T>/VolatilePtr<T,P> structures #124106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
davidwrighton wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Reduce risk of the Volatile<T>/VolatilePtr<T,P> structures #124106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
51b3473
Reduce risk of the Volatile<T>/VolatilePtr<T,P> structures
davidwrighton Feb 6, 2026
6b01a8d
Fix Linux X64 build
davidwrighton Feb 6, 2026
8c8f13a
Copilot fixes + another build fix
davidwrighton Feb 6, 2026
615b934
Remove arithmetic operators from Volatile<T>
davidwrighton Feb 7, 2026
808bd08
Merge branch 'AdjustVolatileUsage' of https://github.com/davidwrighto…
davidwrighton Feb 7, 2026
30fcea1
Fix missed operator usage
davidwrighton Feb 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 26 additions & 36 deletions src/coreclr/gc/env/volatile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,18 +311,16 @@ class Volatile
//
// Allow initialization of Volatile<T> from a T
//
inline Volatile(const T& val)
inline explicit Volatile(const T& val)
{
((volatile T &)m_val) = val;
}

//
// Copy constructor
// Copy/Move constructor deleted
//
inline Volatile(const Volatile<T>& other)
{
((volatile T &)m_val) = other.Load();
}
Volatile(const Volatile<T>& other) = delete;
Volatile(Volatile<T>&& other) = delete;

//
// Loads the value of the volatile variable. See code:VolatileLoad for the semantics of this operation.
Expand Down Expand Up @@ -386,6 +384,8 @@ class Volatile
// Assignment from T
//
inline Volatile<T>& operator=(T val) {Store(val); return *this;}
inline Volatile<T>& operator=(const Volatile<T> & val) {Store(val.Load()); return *this;}
inline Volatile<T>& operator=(Volatile<T> && val) = delete;

//
// Get the address of the volatile variable. This is dangerous, as it allows the value of the
Expand All @@ -409,31 +409,7 @@ class Volatile
//
// Miscellaneous operators. Add more as necessary.
//
inline Volatile<T>& operator+=(T val) {Store(this->Load() + val); return *this;}
inline Volatile<T>& operator-=(T val) {Store(this->Load() - val); return *this;}
inline Volatile<T>& operator|=(T val) {Store(this->Load() | val); return *this;}
inline Volatile<T>& operator&=(T val) {Store(this->Load() & val); return *this;}
inline bool operator!() const { return !this->Load();}

//
// Prefix increment
//
inline Volatile& operator++() {this->Store(this->Load()+1); return *this;}

//
// Postfix increment
//
inline T operator++(int) {T val = this->Load(); this->Store(val+1); return val;}

//
// Prefix decrement
//
inline Volatile& operator--() {this->Store(this->Load()-1); return *this;}

//
// Postfix decrement
//
inline T operator--(int) {T val = this->Load(); this->Store(val-1); return val;}
};

//
Expand All @@ -456,19 +432,23 @@ class VolatilePtr : public Volatile<P>
}

//
// Allow assignment from the pointer type.
// Allow construction from the pointer type and from various representations of NULL; HOWEVER, note that construction must be explicit since
// these constructions do not do a volatile store.
//
inline VolatilePtr(P val) : Volatile<P>(val)
inline explicit VolatilePtr(P val) : Volatile<P>(val)
{
}

//
// Copy constructor
//
inline VolatilePtr(const VolatilePtr& other) : Volatile<P>(other)
inline explicit VolatilePtr(std::nullptr_t val) : Volatile<P>((P)val)
{
}

//
// Copy/Move constructors deleted
//
VolatilePtr(const VolatilePtr& other) = delete;
VolatilePtr(VolatilePtr&& other) = delete;

//
// Cast to the pointer type
//
Expand All @@ -477,6 +457,15 @@ class VolatilePtr : public Volatile<P>
return (P)this->Load();
}

//
// 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=(std::nullptr_t val) {this->Store((P)val); return *this;}
Comment on lines +462 to +467
Copy link

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
//
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;}

Copilot uses AI. Check for mistakes.

//
// Member access
//
Expand Down Expand Up @@ -504,5 +493,6 @@ class VolatilePtr : public Volatile<P>
};

#define VOLATILE(T) Volatile<T>
#define VOLATILE_INIT(val) (val)

#endif //_VOLATILE_H_
50 changes: 31 additions & 19 deletions src/coreclr/gc/gc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ uint8_t g_build_variant = 1;
uint8_t g_build_variant = 2;
#endif //BUILDENV_DEBUG

VOLATILE(int32_t) g_no_gc_lock = -1;
VOLATILE(int32_t) g_no_gc_lock VOLATILE_INIT(-1);

#ifdef TRACE_GC
const char * const allocation_state_str[] = {
Expand Down Expand Up @@ -467,7 +467,7 @@ int gc_heap::gchist_index = 0;
gc_mechanisms_store gc_heap::gchist[max_history_count];

#ifndef MULTIPLE_HEAPS
VOLATILE(bgc_state) gc_heap::current_bgc_state = bgc_not_in_process;
VOLATILE(bgc_state) gc_heap::current_bgc_state(bgc_not_in_process);
int gc_heap::gchist_index_per_heap = 0;
gc_heap::gc_history gc_heap::gchist_per_heap[max_history_count];
#endif //MULTIPLE_HEAPS
Expand Down Expand Up @@ -2456,12 +2456,12 @@ last_recorded_gc_info gc_heap::last_full_blocking_gc_info;

uint64_t gc_heap::last_alloc_reset_suspended_end_time = 0;
size_t gc_heap::max_peak_heap_size = 0;
VOLATILE(size_t) gc_heap::llc_size = 0;
VOLATILE(size_t) gc_heap::llc_size(0);

#ifdef BACKGROUND_GC
last_recorded_gc_info gc_heap::last_bgc_info[2];
VOLATILE(bool) gc_heap::is_last_recorded_bgc = false;
VOLATILE(int) gc_heap::last_bgc_info_index = 0;
VOLATILE(bool) gc_heap::is_last_recorded_bgc(false);
VOLATILE(int) gc_heap::last_bgc_info_index(0);
#endif //BACKGROUND_GC

#ifdef DYNAMIC_HEAP_COUNT
Expand All @@ -2473,7 +2473,7 @@ size_t gc_heap::bgc_th_count_created = 0;
size_t gc_heap::bgc_th_count_created_th_existed = 0;
size_t gc_heap::bgc_th_count_creation_failed = 0;
size_t gc_heap::bgc_init_gc_index = 0;
VOLATILE(short) gc_heap::bgc_init_n_heaps = 0;
VOLATILE(short) gc_heap::bgc_init_n_heaps(0);
size_t gc_heap::hc_change_cancelled_count_bgc = 0;
#endif //BACKGROUND_GC
#endif //DYNAMIC_HEAP_COUNT
Expand Down Expand Up @@ -2556,9 +2556,9 @@ BOOL gc_heap::do_concurrent_p = FALSE;

size_t gc_heap::ephemeral_fgc_counts[max_generation];

VOLATILE(c_gc_state) gc_heap::current_c_gc_state = c_gc_state_free;
VOLATILE(c_gc_state) gc_heap::current_c_gc_state(c_gc_state_free);

VOLATILE(BOOL) gc_heap::gc_background_running = FALSE;
VOLATILE(BOOL) gc_heap::gc_background_running(FALSE);
#endif //BACKGROUND_GC

#ifdef USE_REGIONS
Expand Down Expand Up @@ -3010,7 +3010,7 @@ GCSpinLock gc_heap::more_space_lock_soh;
GCSpinLock gc_heap::more_space_lock_uoh;

#ifdef BACKGROUND_GC
VOLATILE(int32_t) gc_heap::uoh_alloc_thread_count = 0;
VOLATILE(int32_t) gc_heap::uoh_alloc_thread_count(0);
#endif //BACKGROUND_GC

#ifdef SYNCHRONIZATION_STATS
Expand Down Expand Up @@ -8127,7 +8127,7 @@ void gc_heap::fix_allocation_context_heaps (gc_alloc_context* gc_context, void*)
alloc_hp_num %= gc_heap::n_heaps;
acontext->set_alloc_heap (GCHeap::GetHeap (alloc_hp_num));
gc_heap* hp = acontext->get_alloc_heap ()->pGenGCHeap;
hp->alloc_context_count++;
hp->alloc_context_count = hp->alloc_context_count + 1;
}
}

Expand Down Expand Up @@ -19221,7 +19221,7 @@ void gc_heap::balance_heaps (alloc_context* acontext)
acontext->set_home_heap (GCHeap::GetHeap (home_hp_num));
gc_heap* hp = acontext->get_home_heap ()->pGenGCHeap;
acontext->set_alloc_heap (acontext->get_home_heap ());
hp->alloc_context_count++;
hp->alloc_context_count = hp->alloc_context_count + 1;

#ifdef HEAP_BALANCE_INSTRUMENTATION
uint16_t ideal_proc_no = 0;
Expand Down Expand Up @@ -19474,8 +19474,16 @@ void gc_heap::balance_heaps (alloc_context* acontext)
{
final_alloc_hp_num = max_hp->heap_number;

org_hp->alloc_context_count--;
max_hp->alloc_context_count++;
// update the alloc_context_count for the original and new heaps.
// NOTE: at this time the alloc_context_count for these heaps could have changed due to racing threads,
// but we will update the counts based on what we observed, without trying to re-check or
// synchronize, as this is just a heuristic to improve our balancing, and doesn't need to
// be perfectly accurate.
//
// This has been changed from a increment/decrement operators to emphasize that these are not
// atomic changes, so alloc_context_count may be inaccurate.
org_hp->alloc_context_count = org_hp->alloc_context_count - 1;
max_hp->alloc_context_count = max_hp->alloc_context_count + 1;

acontext->set_alloc_heap (GCHeap::GetHeap (final_alloc_hp_num));
if (!gc_thread_no_affinitize_p)
Expand Down Expand Up @@ -21679,9 +21687,13 @@ int gc_heap::generation_to_condemn (int n_initial,
BOOL* elevation_requested_p,
BOOL check_only_p)
{
gc_mechanisms temp_settings = settings;
gc_mechanisms temp_settings;
gen_to_condemn_tuning temp_condemn_reasons;
gc_mechanisms* local_settings = (check_only_p ? &temp_settings : &settings);
if (check_only_p)
{
temp_settings = settings;
}
gen_to_condemn_tuning* local_condemn_reasons = (check_only_p ? &temp_condemn_reasons : &gen_to_condemn_reasons);
if (!check_only_p)
{
Expand Down Expand Up @@ -24237,7 +24249,7 @@ void gc_heap::pm_full_gc_init_or_clear()
// Although arguably we should just turn off PM then...
//assert (settings.entry_memory_load >= high_memory_load_th);
assert (settings.entry_memory_load > 0);
settings.gc_index += 1;
settings.gc_index = settings.gc_index + 1;
do_pre_gc();
}
}
Expand Down Expand Up @@ -29494,8 +29506,8 @@ void gc_heap::process_mark_overflow_internal (int condemned_gen_number,
// achieved via two shared booleans (defined below). These both act as latches that are transitioned only from
// false -> true by unsynchronized code. They are only read or reset to false by a single thread under the
// protection of a join.
static VOLATILE(BOOL) s_fUnpromotedHandles = FALSE;
static VOLATILE(BOOL) s_fUnscannedPromotions = FALSE;
static VOLATILE(BOOL) s_fUnpromotedHandles(FALSE);
static VOLATILE(BOOL) s_fUnscannedPromotions(FALSE);
static VOLATILE(BOOL) s_fScanRequired;
void gc_heap::scan_dependent_handles (int condemned_gen_number, ScanContext *sc, BOOL initial_scan_p)
{
Expand Down Expand Up @@ -47849,14 +47861,14 @@ void gc_heap::descr_generations (const char* msg)
//-----------------------------------------------------------------------------

//Static member variables.
VOLATILE(BOOL) GCHeap::GcInProgress = FALSE;
VOLATILE(BOOL) GCHeap::GcInProgress(FALSE);
GCEvent *GCHeap::WaitForGCEvent = NULL;
unsigned GCHeap::GcCondemnedGeneration = 0;
size_t GCHeap::totalSurvivedSize = 0;
#ifdef FEATURE_PREMORTEM_FINALIZATION
CFinalize* GCHeap::m_Finalize = 0;
BOOL GCHeap::GcCollectClasses = FALSE;
VOLATILE(int32_t) GCHeap::m_GCFLock = 0;
VOLATILE(int32_t) GCHeap::m_GCFLock(0);

#ifndef FEATURE_NATIVEAOT // NativeAOT forces relocation a different way
#ifdef STRESS_HEAP
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/gc/gccommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ uint8_t* g_shadow_lowest_address = NULL;

uint32_t* g_gc_card_table;

VOLATILE(int32_t) g_fSuspensionPending = 0;
VOLATILE(int32_t) g_fSuspensionPending VOLATILE_INIT(0);

#ifdef FEATURE_MANUALLY_MANAGED_CARD_BUNDLES
uint32_t* g_gc_card_bundle_table;
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/gc/gceventstatus.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
#include "common.h"
#include "gceventstatus.h"

Volatile<GCEventLevel> GCEventStatus::enabledLevels[2] = {GCEventLevel_None, GCEventLevel_None};
Volatile<GCEventKeyword> GCEventStatus::enabledKeywords[2] = {GCEventKeyword_None, GCEventKeyword_None};
Volatile<GCEventLevel> GCEventStatus::enabledLevels[2] = {Volatile<GCEventLevel>(GCEventLevel_None), Volatile<GCEventLevel>(GCEventLevel_None)};
Volatile<GCEventKeyword> GCEventStatus::enabledKeywords[2] = {Volatile<GCEventKeyword>(GCEventKeyword_None), Volatile<GCEventKeyword>(GCEventKeyword_None)};
6 changes: 6 additions & 0 deletions src/coreclr/gc/gcpriv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,12 @@ class gc_mechanisms
BOOL stress_induced;
#endif // STRESS_HEAP

gc_mechanisms() = default;
gc_mechanisms(const gc_mechanisms& other)
{
*this = other;
}

// These are opportunistically set
uint32_t entry_memory_load;
uint64_t entry_available_physical_mem;
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/gc/gcscan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#include "gc.h"
#include "objecthandle.h"

VOLATILE(int32_t) GCScan::m_GcStructuresInvalidCnt = 1;
VOLATILE(int32_t) GCScan::m_GcStructuresInvalidCnt VOLATILE_INIT(1);

bool GCScan::GetGcRuntimeStructuresValid ()
{
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/gc/objecthandle.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1680,7 +1680,7 @@ void Ref_CheckAlive(uint32_t condemned, uint32_t maxgen, ScanContext *sc)
#endif
}

static VOLATILE(int32_t) uCount = 0;
static VOLATILE(int32_t) uCount VOLATILE_INIT(0);

// NOTE: Please: if you update this function, update the very similar profiling function immediately below!!!
void Ref_UpdatePointers(uint32_t condemned, uint32_t maxgen, ScanContext* sc, Ref_promote_func* fn)
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/inc/daccess.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2230,7 +2230,7 @@ public: name(int dummy) : base(dummy) {}
#define VOLATILE_SVAL_IMPL(type, cls, var) \
Volatile<type> cls::var
#define VOLATILE_SVAL_IMPL_INIT(type, cls, var, init) \
Volatile<type> cls::var = init
Volatile<type> cls::var(init)
#define SVAL_IMPL_NS(type, ns, cls, var) \
type cls::var
#define SVAL_IMPL_NS_INIT(type, ns, cls, var, init) \
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/inc/profilepriv.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ inline void ProfilerInfo::ResetPerSessionStatus()
{
LIMITED_METHOD_CONTRACT;

pProfInterface = NULL;
pProfInterface = nullptr;
eventMask.SetEventMask(COR_PRF_MONITOR_NONE);
eventMask.SetEventMaskHigh(COR_PRF_HIGH_MONITOR_NONE);
}
Expand Down
Loading
Loading