Merge pull request #5483 from YosysHQ/emil/idstring-memory-safety

Resolve IdString memory leaks

kernel/constids.inc

@@ -196,6 +196,7 @@ X($bweqx)
 X($bwmux)
 X($check)
 X($concat)
+X($connect)
 X($cover)
 X($demux)
 X($dff)
@@ -222,6 +223,7 @@ X($get_tag)
 X($gt)
 X($initstate)
 X($input)
+X($input_port)
 X($lcu)
 X($le)
 X($live)

kernel/driver.cc

@@ -158,6 +158,7 @@ extern "C" {
 
 void yosys_atexit()
 {
+	RTLIL::OwningIdString::collect_garbage(false);
 #if defined(YOSYS_ENABLE_READLINE) || defined(YOSYS_ENABLE_EDITLINE)
 	if (!yosys_history_file.empty()) {
 #if defined(YOSYS_ENABLE_READLINE)
@@ -706,11 +707,16 @@ int main(int argc, char **argv)
 
 		for (auto &it : pass_register)
 			if (it.second->call_counter) {
-				total_ns += it.second->runtime_ns + 1;
+				auto pass_ns = it.second->runtime_ns + 1;
-				timedat.insert(make_tuple(it.second->runtime_ns + 1, it.second->call_counter, it.first));
+				total_ns += pass_ns;
+				timedat.insert(make_tuple(pass_ns, it.second->call_counter, it.first));
 			}
-		timedat.insert(make_tuple(RTLIL::OwningIdString::garbage_collection_ns() + 1,
+		{
-				RTLIL::OwningIdString::garbage_collection_count(), "id_gc"));
+			auto gc_ns = RTLIL::OwningIdString::garbage_collection_ns() + 1;
+			total_ns += gc_ns;
+			timedat.insert(make_tuple(gc_ns,
+					RTLIL::OwningIdString::garbage_collection_count(), "id_gc"));
+		}
 
 		if (timing_details)
 		{

kernel/rtlil.cc

@@ -130,7 +130,7 @@ int RTLIL::IdString::really_insert(std::string_view p, std::unordered_map<std::s
 
 #ifdef YOSYS_XTRACE_GET_PUT
 	if (yosys_xtrace)
-		log("#X# GET-BY-NAME '%s' (index %d, refcount %u)\n", global_id_storage_.at(idx), idx, refcount(idx));
+		log("#X# GET-BY-NAME '%s' (index %d, refcount %u)\n", global_id_storage_.at(idx).buf, idx, refcount(idx));
 #endif
 	return idx;
 }
@@ -246,14 +246,15 @@ struct IdStringCollector {
 int64_t RTLIL::OwningIdString::gc_ns;
 int RTLIL::OwningIdString::gc_count;
 
-void RTLIL::OwningIdString::collect_garbage()
+void RTLIL::OwningIdString::collect_garbage(bool trace)
 {
 	int64_t start = PerformanceTimer::query();
 #ifndef YOSYS_NO_IDS_REFCNT
 	IdStringCollector collector;
-	for (auto &[idx, design] : *RTLIL::Design::get_all_designs()) {
+	if (trace)
-		collector.trace(*design);
+		for (auto &[idx, design] : *RTLIL::Design::get_all_designs()) {
-	}
+			collector.trace(*design);
+		}
 	int size = GetSize(global_id_storage_);
 	for (int i = static_cast<int>(StaticId::STATIC_ID_END); i < size; ++i) {
 		RTLIL::IdString::Storage &storage = global_id_storage_.at(i);

kernel/rtlil.h

@@ -135,10 +135,6 @@ struct RTLIL::IdString
 		std::string_view str_view() const { return {buf, static_cast<size_t>(size)}; }
 	};
 
-	#undef YOSYS_XTRACE_GET_PUT
-	#undef YOSYS_SORT_ID_FREE_LIST
-	#undef YOSYS_NO_IDS_REFCNT
-
 	// the global id string cache
 
 	static bool destruct_guard_ok; // POD, will be initialized to zero
@@ -178,7 +174,7 @@ struct RTLIL::IdString
 			if (global_id_storage_.at(idx).buf == nullptr)
 				log("#X# DB-DUMP index %d: FREE\n", idx);
 			else
-				log("#X# DB-DUMP index %d: '%s' (ref %u)\n", idx, refcount(idx).buf, refcount);
+				log("#X# DB-DUMP index %d: '%s' (ref %u)\n", idx, global_id_storage_.at(idx).buf, refcount(idx));
 		}
 	#endif
 	}
@@ -578,7 +574,7 @@ struct RTLIL::OwningIdString : public RTLIL::IdString {
 	}
 
 	// Collect all non-owning references.
-	static void collect_garbage();
+	static void collect_garbage(bool trace = true);
 	static int64_t garbage_collection_ns() { return gc_ns; }
 	static int garbage_collection_count() { return gc_count; }
 

techlibs/ice40/ice40_opt.cc

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

techlibs/ice40/ice40_wrapcarry.cc

@@ -62,7 +62,7 @@ void create_ice40_wrapcarry(ice40_wrapcarry_pm &pm)
 		cell->attributes[stringf("\\SB_CARRY.%s", a.first)] = a.second;
 	for (const auto &a : st.lut->attributes)
 		cell->attributes[stringf("\\SB_LUT4.%s", a.first)] = a.second;
-	cell->attributes[ID(SB_LUT4.name)] = Const(st.lut->name.str());
+	cell->attributes[IdString{"\\SB_LUT4.name"}] = Const(st.lut->name.str());
 	if (st.carry->get_bool_attribute(ID::keep) || st.lut->get_bool_attribute(ID::keep))
 		cell->attributes[ID::keep] = true;
 
@@ -122,7 +122,7 @@ struct Ice40WrapCarryPass : public Pass {
 					carry->setPort(ID::CI, cell->getPort(ID::CI));
 					carry->setPort(ID::CO, cell->getPort(ID::CO));
 					module->swap_names(carry, cell);
-					auto lut_name = cell->attributes.at(ID(SB_LUT4.name), Const(NEW_ID.str())).decode_string();
+					auto lut_name = cell->attributes.at(IdString{"\\SB_LUT4.name"}, Const(NEW_ID.str())).decode_string();
 					auto lut = module->addCell(lut_name, ID($lut));
 					lut->setParam(ID::WIDTH, 4);
 					lut->setParam(ID::LUT, cell->getParam(ID::LUT));
@@ -138,7 +138,7 @@ struct Ice40WrapCarryPass : public Pass {
 							lut->attributes[a.first.c_str() + strlen("\\SB_LUT4.")] = a.second;
 						else if (a.first == ID::src)
 							src = a.second;
-						else if (a.first.in(ID(SB_LUT4.name), ID::keep, ID::module_not_derived))
+						else if (a.first.in(IdString{"\\SB_LUT4.name"}, ID::keep, ID::module_not_derived))
 							continue;
 						else
 							log_abort();

techlibs/microchip/microchip_dsp.pmg

@@ -268,7 +268,7 @@ endmatch
 code
 	if (postAdd)
 	{
-		if (postAdd->type.in(ID($sub)) && postAddAB == \A) {
+		if (postAdd->type.in($sub) && postAddAB == \A) {
 			// if $sub, the multiplier output must match to $sub.B, otherwise no match	
 		} else {
 			u_postAddAB = postAddAB;

techlibs/microchip/microchip_dsp_cascade.pmg

@@ -115,9 +115,9 @@ finally
 				Wire *cascade = module->addWire(NEW_ID, 48);
 
 				// zero port C and move wire to cascade
-				dsp_pcin->setPort(ID(C), Const(0, 48));
+				dsp_pcin->setPort(\C, Const(0, 48));
-				dsp_pcin->setPort(ID(CDIN), cascade);
+				dsp_pcin->setPort(\CDIN, cascade);
-				dsp->setPort(ID(CDOUT), cascade);
+				dsp->setPort(\CDOUT, cascade);
 
 				// Configure wire to cascade the dsps
 				add_siguser(cascade, dsp_pcin);

techlibs/xilinx/xilinx_dsp_cascade.pmg

@@ -90,9 +90,9 @@ finally
 			if (i % MAX_DSP_CASCADE > 0) {
 				if (P >= 0) {
 					Wire *cascade = module->addWire(NEW_ID, 48);
-					dsp_pcin->setPort(ID(C), Const(0, 48));
+					dsp_pcin->setPort(\C, Const(0, 48));
-					dsp_pcin->setPort(ID(PCIN), cascade);
+					dsp_pcin->setPort(\PCIN, cascade);
-					dsp->setPort(ID(PCOUT), cascade);
+					dsp->setPort(\PCOUT, cascade);
 					add_siguser(cascade, dsp_pcin);
 					add_siguser(cascade, dsp);
 
@@ -118,15 +118,15 @@ finally
 				}
 				if (AREG >= 0) {
 					Wire *cascade = module->addWire(NEW_ID, 30);
-					dsp_pcin->setPort(ID(A), Const(0, 30));
+					dsp_pcin->setPort(\A, Const(0, 30));
-					dsp_pcin->setPort(ID(ACIN), cascade);
+					dsp_pcin->setPort(\ACIN, cascade);
-					dsp->setPort(ID(ACOUT), cascade);
+					dsp->setPort(\ACOUT, cascade);
 					add_siguser(cascade, dsp_pcin);
 					add_siguser(cascade, dsp);
 
 					if (dsp->type.in(\DSP48E1))
-						dsp->setParam(ID(ACASCREG), AREG);
+						dsp->setParam(\ACASCREG, AREG);
-					dsp_pcin->setParam(ID(A_INPUT), Const("CASCADE"));
+					dsp_pcin->setParam(\A_INPUT, Const("CASCADE"));
 
 					log_debug("ACOUT -> ACIN cascade for %s -> %s\n", log_id(dsp), log_id(dsp_pcin));
 				}
@@ -138,18 +138,18 @@ finally
 						//   BCOUT from an adjacent DSP48A1 slice. The tools then
 						//   translate BCOUT cascading to the dedicated BCIN input
 						//   and set the B_INPUT attribute for implementation."
-						dsp_pcin->setPort(ID(B), cascade);
+						dsp_pcin->setPort(\B, cascade);
 					}
 					else {
-						dsp_pcin->setPort(ID(B), Const(0, 18));
+						dsp_pcin->setPort(\B, Const(0, 18));
-						dsp_pcin->setPort(ID(BCIN), cascade);
+						dsp_pcin->setPort(\BCIN, cascade);
 					}
-					dsp->setPort(ID(BCOUT), cascade);
+					dsp->setPort(\BCOUT, cascade);
 					add_siguser(cascade, dsp_pcin);
 					add_siguser(cascade, dsp);
 
 					if (dsp->type.in(\DSP48E1)) {
-						dsp->setParam(ID(BCASCREG), BREG);
+						dsp->setParam(\BCASCREG, BREG);
 						// According to UG389 p13 [https://www.xilinx.com/support/documentation/user_guides/ug389.pdf]
 						// "The attribute is only used by place and route tools and
 						//   is not necessary for the users to set for synthesis. The
@@ -158,7 +158,7 @@ finally
 						//   BCOUT of another DSP48A1 slice, then the tools automatically
 						//   set the attribute to 'CASCADE', otherwise it is set to
 						//   'DIRECT'".
-						dsp_pcin->setParam(ID(B_INPUT), Const("CASCADE"));
+						dsp_pcin->setParam(\B_INPUT, Const("CASCADE"));
 					}
 
 					log_debug("BCOUT -> BCIN cascade for %s -> %s\n", log_id(dsp), log_id(dsp_pcin));