Skip to content

Commit 4c73877

Browse files
committed
Fix memory leak and thread safety in synthetic SPVVariable handling
- Replace malloc + placement new with unique_ptr for synthetic SPVVariable objects created for device-only variables (templated __constant__ vars) - Add mutex protection around static SyntheticVars vector access - Add duplicate check to prevent concurrent threads from creating same var - Remove unreachable dead code in SPVRegister::bindVariable that checked for special abort/heap variables after already returning for same-name case
1 parent 23b900f commit 4c73877

2 files changed

Lines changed: 50 additions & 39 deletions

File tree

src/CHIPBackend.cc

Lines changed: 48 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1185,51 +1185,66 @@ chipstar::Module *chipstar::Device::getOrCreateModule(HostPtr Ptr) {
11851185

11861186
// Discover device-only variables (e.g., template instantiations) that weren't
11871187
// registered via __hipRegisterVar. These have shadow kernels but no host symbol.
1188+
// Static variables protected by DeviceVarMtx to ensure thread safety.
11881189
static int DummyHostPtr = 0;
11891190
// Use unique_ptr for automatic cleanup when the program exits
11901191
static std::vector<std::unique_ptr<SPVVariable>> SyntheticVars;
11911192

1192-
for (auto *Kernel : Mod->getKernels()) {
1193-
const std::string &KernelName = Kernel->getName();
1194-
size_t PrefixLen = strlen(ChipVarInfoPrefix);
1195-
1196-
// Check if this is a variable info shadow kernel
1197-
if (KernelName.length() <= PrefixLen ||
1198-
KernelName.substr(0, PrefixLen) != ChipVarInfoPrefix)
1199-
continue;
1193+
{
1194+
LOCK(DeviceVarMtx); // Protect static SyntheticVars from concurrent access
1195+
for (auto *Kernel : Mod->getKernels()) {
1196+
const std::string &KernelName = Kernel->getName();
1197+
size_t PrefixLen = strlen(ChipVarInfoPrefix);
1198+
1199+
// Check if this is a variable info shadow kernel
1200+
if (KernelName.length() <= PrefixLen ||
1201+
KernelName.substr(0, PrefixLen) != ChipVarInfoPrefix)
1202+
continue;
1203+
1204+
// Extract variable name
1205+
std::string VarName = KernelName.substr(PrefixLen);
1206+
1207+
// Check if we already processed this variable
1208+
bool AlreadyRegistered = false;
1209+
for (const auto &Info : SrcMod->Variables) {
1210+
std::string NameTmp(Info.Name.begin(), Info.Name.end());
1211+
if (NameTmp == VarName) {
1212+
AlreadyRegistered = true;
1213+
break;
1214+
}
1215+
}
12001216

1201-
// Extract variable name
1202-
std::string VarName = KernelName.substr(PrefixLen);
1217+
if (AlreadyRegistered)
1218+
continue;
12031219

1204-
// Check if we already processed this variable
1205-
bool AlreadyRegistered = false;
1206-
for (const auto &Info : SrcMod->Variables) {
1207-
std::string NameTmp(Info.Name.begin(), Info.Name.end());
1208-
if (NameTmp == VarName) {
1209-
AlreadyRegistered = true;
1210-
break;
1220+
// Check if already in SyntheticVars (another thread may have added it)
1221+
bool AlreadySynthesized = false;
1222+
for (const auto &SV : SyntheticVars) {
1223+
if (SV->Name == VarName) {
1224+
AlreadySynthesized = true;
1225+
break;
1226+
}
12111227
}
1212-
}
1228+
if (AlreadySynthesized)
1229+
continue;
12131230

1214-
if (AlreadyRegistered)
1215-
continue;
1216-
1217-
// This is a device-only variable - create a DeviceVar for it without a host pointer
1218-
logTrace("Found device-only variable: {} (no host symbol)", VarName);
1231+
// This is a device-only variable - create a DeviceVar for it
1232+
logTrace("Found device-only variable: {} (no host symbol)", VarName);
12191233

1220-
// Create a synthetic SPVVariable for this device-only variable
1221-
// Use aggregate initialization since SPVVariable has no default constructor
1222-
auto *RawVar = new SPVVariable{
1223-
{const_cast<SPVModule *>(SrcMod), HostPtr(&DummyHostPtr), VarName}, 0};
1224-
std::unique_ptr<SPVVariable> SyntheticVar(RawVar);
1234+
// Create a synthetic SPVVariable for this device-only variable
1235+
// Use aggregate initialization since SPVVariable has no default constructor
1236+
auto *RawVar = new SPVVariable{
1237+
{const_cast<SPVModule *>(SrcMod), HostPtr(&DummyHostPtr), VarName}, 0};
1238+
std::unique_ptr<SPVVariable> SyntheticVar(RawVar);
12251239

1226-
auto *Var = new chipstar::DeviceVar(SyntheticVar.get());
1227-
Mod->addDeviceVariable(Var);
1240+
auto *Var = new chipstar::DeviceVar(SyntheticVar.get());
1241+
Mod->addDeviceVariable(Var);
12281242

1229-
// Store the synthetic variable so it persists (unique_ptr handles cleanup)
1230-
SyntheticVars.push_back(std::move(SyntheticVar));
1243+
// Store the synthetic variable so it persists (unique_ptr handles cleanup)
1244+
SyntheticVars.push_back(std::move(SyntheticVar));
12311245

1232-
// Note: We don't add to DeviceVarLookup_ since there's no host pointer to look up
1246+
// Note: We don't add to DeviceVarLookup_ since there's no host pointer
1247+
}
12331248
}
12341249

12351250
#ifndef NDEBUG

src/SPVRegister.cc

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -125,15 +125,11 @@ void SPVRegister::bindVariable(SPVRegister::Handle Handle, HostPtr Ptr,
125125
// In case of **templated** and **inline qualified** __constant__
126126
// variables, there may be duplicate __hipRegisterVar() calls.
127127
// Similar to functions, record the first one and ignore duplicates.
128+
// This also handles special abort/heap variables which may have
129+
// multiple registrations with the same host pointer and name.
128130
if (HostPtrLookup_[Ptr]->Name == Name) {
129131
return;
130132
}
131-
// A variable made for abort() and device-side malloc implementation is an
132-
// exception to this due to the way it's modeled.
133-
if ((Name == ChipDeviceAbortFlagName || Name == ChipDeviceHeapName) &&
134-
HostPtrLookup_[Ptr]->Name == Name) {
135-
return;
136-
}
137133
assert(false && "Host-pointer is already mapped to a different variable.");
138134
}
139135

0 commit comments

Comments
 (0)