Skip to content

Commit 1929956

Browse files
widlarizerwidlarizer
authored
Merge pull request #5483 from YosysHQ/emil/idstring-memory-safety
Resolve IdString memory leaks
2 parents 04135ba + f226364 commit 1929956

File tree

9 files changed

+43
-38
lines changed

9 files changed

+43
-38
lines changed

‎kernel/constids.inc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,7 @@ X($bweqx)
196196
X($bwmux)
197197
X($check)
198198
X($concat)
199+
X($connect)
199200
X($cover)
200201
X($demux)
201202
X($dff)
@@ -222,6 +223,7 @@ X($get_tag)
222223
X($gt)
223224
X($initstate)
224225
X($input)
226+
X($input_port)
225227
X($lcu)
226228
X($le)
227229
X($live)

‎kernel/driver.cc‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,7 @@ extern "C" {
158158

159159
void yosys_atexit()
160160
{
161+
RTLIL::OwningIdString::collect_garbage(false);
161162
#if defined(YOSYS_ENABLE_READLINE) || defined(YOSYS_ENABLE_EDITLINE)
162163
if (!yosys_history_file.empty()) {
163164
#if defined(YOSYS_ENABLE_READLINE)
@@ -706,11 +707,16 @@ int main(int argc, char **argv)
706707

707708
for (auto &it : pass_register)
708709
if (it.second->call_counter) {
709-
total_ns += it.second->runtime_ns + 1;
710-
timedat.insert(make_tuple(it.second->runtime_ns + 1, it.second->call_counter, it.first));
710+
auto pass_ns = it.second->runtime_ns + 1;
711+
total_ns += pass_ns;
712+
timedat.insert(make_tuple(pass_ns, it.second->call_counter, it.first));
711713
}
712-
timedat.insert(make_tuple(RTLIL::OwningIdString::garbage_collection_ns() + 1,
713-
RTLIL::OwningIdString::garbage_collection_count(), "id_gc"));
714+
{
715+
auto gc_ns = RTLIL::OwningIdString::garbage_collection_ns() + 1;
716+
total_ns += gc_ns;
717+
timedat.insert(make_tuple(gc_ns,
718+
RTLIL::OwningIdString::garbage_collection_count(), "id_gc"));
719+
}
714720

715721
if (timing_details)
716722
{

‎kernel/rtlil.cc‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@ int RTLIL::IdString::really_insert(std::string_view p, std::unordered_map<std::s
130130

131131
#ifdef YOSYS_XTRACE_GET_PUT
132132
if (yosys_xtrace)
133-
log("#X# GET-BY-NAME '%s' (index %d, refcount %u)\n", global_id_storage_.at(idx), idx, refcount(idx));
133+
log("#X# GET-BY-NAME '%s' (index %d, refcount %u)\n", global_id_storage_.at(idx).buf, idx, refcount(idx));
134134
#endif
135135
return idx;
136136
}
@@ -246,14 +246,15 @@ struct IdStringCollector {
246246
int64_t RTLIL::OwningIdString::gc_ns;
247247
int RTLIL::OwningIdString::gc_count;
248248

249-
void RTLIL::OwningIdString::collect_garbage()
249+
void RTLIL::OwningIdString::collect_garbage(bool trace)
250250
{
251251
int64_t start = PerformanceTimer::query();
252252
#ifndef YOSYS_NO_IDS_REFCNT
253253
IdStringCollector collector;
254-
for (auto &[idx, design] : *RTLIL::Design::get_all_designs()) {
255-
collector.trace(*design);
256-
}
254+
if (trace)
255+
for (auto &[idx, design] : *RTLIL::Design::get_all_designs()) {
256+
collector.trace(*design);
257+
}
257258
int size = GetSize(global_id_storage_);
258259
for (int i = static_cast<int>(StaticId::STATIC_ID_END); i < size; ++i) {
259260
RTLIL::IdString::Storage &storage = global_id_storage_.at(i);

‎kernel/rtlil.h‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -135,10 +135,6 @@ struct RTLIL::IdString
135135
std::string_view str_view() const { return {buf, static_cast<size_t>(size)}; }
136136
};
137137

138-
#undef YOSYS_XTRACE_GET_PUT
139-
#undef YOSYS_SORT_ID_FREE_LIST
140-
#undef YOSYS_NO_IDS_REFCNT
141-
142138
// the global id string cache
143139

144140
static bool destruct_guard_ok; // POD, will be initialized to zero
@@ -178,7 +174,7 @@ struct RTLIL::IdString
178174
if (global_id_storage_.at(idx).buf == nullptr)
179175
log("#X# DB-DUMP index %d: FREE\n", idx);
180176
else
181-
log("#X# DB-DUMP index %d: '%s' (ref %u)\n", idx, refcount(idx).buf, refcount);
177+
log("#X# DB-DUMP index %d: '%s' (ref %u)\n", idx, global_id_storage_.at(idx).buf, refcount(idx));
182178
}
183179
#endif
184180
}
@@ -578,7 +574,7 @@ struct RTLIL::OwningIdString : public RTLIL::IdString {
578574
}
579575

580576
// Collect all non-owning references.
581-
static void collect_garbage();
577+
static void collect_garbage(bool trace = true);
582578
static int64_t garbage_collection_ns() { return gc_ns; }
583579
static int garbage_collection_count() { return gc_count; }
584580

‎techlibs/ice40/ice40_opt.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ static void run_ice40_opts(Module *module)
117117

118118
if (GetSize(replacement_output)) {
119119
optimized_co.insert(sigmap(cell->getPort(ID::CO)[0]));
120-
auto it = cell->attributes.find(ID(SB_LUT4.name));
120+
auto it = cell->attributes.find(IdString{"\\SB_LUT4.name"});
121121
if (it != cell->attributes.end()) {
122122
module->rename(cell, it->second.decode_string());
123123
decltype(Cell::attributes) new_attr;
@@ -126,7 +126,7 @@ static void run_ice40_opts(Module *module)
126126
new_attr[a.first.c_str() + strlen("\\SB_LUT4.")] = a.second;
127127
else if (a.first == ID::src)
128128
new_attr.insert(std::make_pair(a.first, a.second));
129-
else if (a.first.in(ID(SB_LUT4.name), ID::keep, ID::module_not_derived))
129+
else if (a.first.in(IdString{"\\SB_LUT4.name"}, ID::keep, ID::module_not_derived))
130130
continue;
131131
else if (a.first.begins_with("\\SB_CARRY.\\"))
132132
continue;

‎techlibs/ice40/ice40_wrapcarry.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ void create_ice40_wrapcarry(ice40_wrapcarry_pm &pm)
6262
cell->attributes[stringf("\\SB_CARRY.%s", a.first)] = a.second;
6363
for (const auto &a : st.lut->attributes)
6464
cell->attributes[stringf("\\SB_LUT4.%s", a.first)] = a.second;
65-
cell->attributes[ID(SB_LUT4.name)] = Const(st.lut->name.str());
65+
cell->attributes[IdString{"\\SB_LUT4.name"}] = Const(st.lut->name.str());
6666
if (st.carry->get_bool_attribute(ID::keep) || st.lut->get_bool_attribute(ID::keep))
6767
cell->attributes[ID::keep] = true;
6868

@@ -122,7 +122,7 @@ struct Ice40WrapCarryPass : public Pass {
122122
carry->setPort(ID::CI, cell->getPort(ID::CI));
123123
carry->setPort(ID::CO, cell->getPort(ID::CO));
124124
module->swap_names(carry, cell);
125-
auto lut_name = cell->attributes.at(ID(SB_LUT4.name), Const(NEW_ID.str())).decode_string();
125+
auto lut_name = cell->attributes.at(IdString{"\\SB_LUT4.name"}, Const(NEW_ID.str())).decode_string();
126126
auto lut = module->addCell(lut_name, ID($lut));
127127
lut->setParam(ID::WIDTH, 4);
128128
lut->setParam(ID::LUT, cell->getParam(ID::LUT));
@@ -138,7 +138,7 @@ struct Ice40WrapCarryPass : public Pass {
138138
lut->attributes[a.first.c_str() + strlen("\\SB_LUT4.")] = a.second;
139139
else if (a.first == ID::src)
140140
src = a.second;
141-
else if (a.first.in(ID(SB_LUT4.name), ID::keep, ID::module_not_derived))
141+
else if (a.first.in(IdString{"\\SB_LUT4.name"}, ID::keep, ID::module_not_derived))
142142
continue;
143143
else
144144
log_abort();

‎techlibs/microchip/microchip_dsp.pmg‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ endmatch
268268
code
269269
if (postAdd)
270270
{
271-
if (postAdd->type.in(ID($sub)) && postAddAB == \A) {
271+
if (postAdd->type.in($sub) && postAddAB == \A) {
272272
// if $sub, the multiplier output must match to $sub.B, otherwise no match
273273
} else {
274274
u_postAddAB = postAddAB;

‎techlibs/microchip/microchip_dsp_cascade.pmg‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,9 +115,9 @@ finally
115115
Wire *cascade = module->addWire(NEW_ID, 48);
116116

117117
// zero port C and move wire to cascade
118-
dsp_pcin->setPort(ID(C), Const(0, 48));
119-
dsp_pcin->setPort(ID(CDIN), cascade);
120-
dsp->setPort(ID(CDOUT), cascade);
118+
dsp_pcin->setPort(\C, Const(0, 48));
119+
dsp_pcin->setPort(\CDIN, cascade);
120+
dsp->setPort(\CDOUT, cascade);
121121

122122
// Configure wire to cascade the dsps
123123
add_siguser(cascade, dsp_pcin);

‎techlibs/xilinx/xilinx_dsp_cascade.pmg‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,9 @@ finally
9090
if (i % MAX_DSP_CASCADE > 0) {
9191
if (P >= 0) {
9292
Wire *cascade = module->addWire(NEW_ID, 48);
93-
dsp_pcin->setPort(ID(C), Const(0, 48));
94-
dsp_pcin->setPort(ID(PCIN), cascade);
95-
dsp->setPort(ID(PCOUT), cascade);
93+
dsp_pcin->setPort(\C, Const(0, 48));
94+
dsp_pcin->setPort(\PCIN, cascade);
95+
dsp->setPort(\PCOUT, cascade);
9696
add_siguser(cascade, dsp_pcin);
9797
add_siguser(cascade, dsp);
9898

@@ -118,15 +118,15 @@ finally
118118
}
119119
if (AREG >= 0) {
120120
Wire *cascade = module->addWire(NEW_ID, 30);
121-
dsp_pcin->setPort(ID(A), Const(0, 30));
122-
dsp_pcin->setPort(ID(ACIN), cascade);
123-
dsp->setPort(ID(ACOUT), cascade);
121+
dsp_pcin->setPort(\A, Const(0, 30));
122+
dsp_pcin->setPort(\ACIN, cascade);
123+
dsp->setPort(\ACOUT, cascade);
124124
add_siguser(cascade, dsp_pcin);
125125
add_siguser(cascade, dsp);
126126

127127
if (dsp->type.in(\DSP48E1))
128-
dsp->setParam(ID(ACASCREG), AREG);
129-
dsp_pcin->setParam(ID(A_INPUT), Const("CASCADE"));
128+
dsp->setParam(\ACASCREG, AREG);
129+
dsp_pcin->setParam(\A_INPUT, Const("CASCADE"));
130130

131131
log_debug("ACOUT -> ACIN cascade for %s -> %s\n", log_id(dsp), log_id(dsp_pcin));
132132
}
@@ -138,18 +138,18 @@ finally
138138
// BCOUT from an adjacent DSP48A1 slice. The tools then
139139
// translate BCOUT cascading to the dedicated BCIN input
140140
// and set the B_INPUT attribute for implementation."
141-
dsp_pcin->setPort(ID(B), cascade);
141+
dsp_pcin->setPort(\B, cascade);
142142
}
143143
else {
144-
dsp_pcin->setPort(ID(B), Const(0, 18));
145-
dsp_pcin->setPort(ID(BCIN), cascade);
144+
dsp_pcin->setPort(\B, Const(0, 18));
145+
dsp_pcin->setPort(\BCIN, cascade);
146146
}
147-
dsp->setPort(ID(BCOUT), cascade);
147+
dsp->setPort(\BCOUT, cascade);
148148
add_siguser(cascade, dsp_pcin);
149149
add_siguser(cascade, dsp);
150150

151151
if (dsp->type.in(\DSP48E1)) {
152-
dsp->setParam(ID(BCASCREG), BREG);
152+
dsp->setParam(\BCASCREG, BREG);
153153
// According to UG389 p13 [https://www.xilinx.com/support/documentation/user_guides/ug389.pdf]
154154
// "The attribute is only used by place and route tools and
155155
// is not necessary for the users to set for synthesis. The
@@ -158,7 +158,7 @@ finally
158158
// BCOUT of another DSP48A1 slice, then the tools automatically
159159
// set the attribute to 'CASCADE', otherwise it is set to
160160
// 'DIRECT'".
161-
dsp_pcin->setParam(ID(B_INPUT), Const("CASCADE"));
161+
dsp_pcin->setParam(\B_INPUT, Const("CASCADE"));
162162
}
163163

164164
log_debug("BCOUT -> BCIN cascade for %s -> %s\n", log_id(dsp), log_id(dsp_pcin));

0 commit comments

Comments
 (0)