Merge pull request #5514 from rocallahan/idstring-safety

Make `IdString` thread-safe for read-only access

frontends/blif/blifparse.cc

@@ -245,7 +245,7 @@ void parse_blif(RTLIL::Design *design, std::istream &f, IdString dff_name, bool
 					if (undef_wire != nullptr)
 						module->rename(undef_wire, stringf("$undef$%d", ++blif_maxnum));
 
-					autoidx = std::max(autoidx, blif_maxnum+1);
+					autoidx.ensure_at_least(blif_maxnum+1);
 					blif_maxnum = 0;
 				}
 

kernel/log.cc

@@ -720,7 +720,7 @@ dict<std::string, std::pair<std::string, int>> get_coverage_data()
 		if (coverage_data.count(p->id))
 			log_warning("found duplicate coverage id \"%s\".\n", p->id);
 		coverage_data[p->id].first = stringf("%s:%d:%s", p->file, p->line, p->func);
-		coverage_data[p->id].second += p->counter;
+		coverage_data[p->id].second += p->counter.load(std::memory_order_relaxed);
 	}
 
 	for (auto &it : coverage_data)

kernel/log.h

@@ -24,6 +24,7 @@
 
 #include <time.h>
 
+#include <atomic>
 #include <regex>
 #define YS_REGEX_COMPILE(param) std::regex(param, \
 				std::regex_constants::nosubs | \
@@ -298,15 +299,16 @@ void log_abort_internal(const char *file, int line);
 
 #define cover(_id) do { \
     static CoverData __d __attribute__((section("yosys_cover_list"), aligned(1), used)) = { __FILE__, __FUNCTION__, _id, __LINE__, 0 }; \
-    __d.counter++; \
+    __d.counter.fetch_add(1, std::memory_order_relaxed); \
 } while (0)
 
 struct CoverData {
 	const char *file, *func, *id;
-	int line, counter;
-} YS_ATTRIBUTE(packed);
+	int line;
+	std::atomic<int> counter;
+};
 
-// this two symbols are created by the linker for the "yosys_cover_list" ELF section
+// this two symbols are created by the linker __start_yosys_cover_listfor the "yosys_cover_list" ELF section
 extern "C" struct CoverData __start_yosys_cover_list[];
 extern "C" struct CoverData __stop_yosys_cover_list[];
 

kernel/rtlil.cc

@@ -38,12 +38,9 @@ bool RTLIL::IdString::destruct_guard_ok = false;
 RTLIL::IdString::destruct_guard_t RTLIL::IdString::destruct_guard;
 std::vector<RTLIL::IdString::Storage> RTLIL::IdString::global_id_storage_;
 std::unordered_map<std::string_view, int> RTLIL::IdString::global_id_index_;
-std::unordered_map<int, const std::string*> RTLIL::IdString::global_autoidx_id_prefix_storage_;
-std::unordered_map<int, char*> RTLIL::IdString::global_autoidx_id_storage_;
-#ifndef YOSYS_NO_IDS_REFCNT
+std::unordered_map<int, RTLIL::IdString::AutoidxStorage> RTLIL::IdString::global_autoidx_id_storage_;
 std::unordered_map<int, int> RTLIL::IdString::global_refcount_storage_;
 std::vector<int> RTLIL::IdString::global_free_idx_list_;
-#endif
 
 static void populate(std::string_view name)
 {
@@ -95,16 +92,16 @@ int RTLIL::IdString::really_insert(std::string_view p, std::unordered_map<std::s
 		size_t autoidx_pos = p.find_last_of('$') + 1;
 		std::optional<int> p_autoidx = parse_autoidx(p.substr(autoidx_pos));
 		if (p_autoidx.has_value()) {
-			auto prefix_it = global_autoidx_id_prefix_storage_.find(-*p_autoidx);
-			if (prefix_it != global_autoidx_id_prefix_storage_.end() && p.substr(0, autoidx_pos) == *prefix_it->second)
+			auto autoidx_it = global_autoidx_id_storage_.find(-*p_autoidx);
+			if (autoidx_it != global_autoidx_id_storage_.end() &&
+					p.substr(0, autoidx_pos) == *autoidx_it->second.prefix)
 				return -*p_autoidx;
 			// Ensure NEW_ID/NEW_ID_SUFFIX will not create collisions with the ID
 			// we're about to create.
-			autoidx = std::max(autoidx, *p_autoidx + 1);
+			autoidx.ensure_at_least(*p_autoidx + 1);
 		}
 	}
 
-#ifndef YOSYS_NO_IDS_REFCNT
 	if (global_free_idx_list_.empty()) {
 		log_assert(global_id_storage_.size() < 0x40000000);
 		global_free_idx_list_.push_back(global_id_storage_.size());
@@ -113,10 +110,6 @@ int RTLIL::IdString::really_insert(std::string_view p, std::unordered_map<std::s
 
 	int idx = global_free_idx_list_.back();
 	global_free_idx_list_.pop_back();
-#else
-	int idx = global_id_storage_.size();
-	global_id_index_[global_id_storage_.back()] = idx;
-#endif
 	char* buf = static_cast<char*>(malloc(p.size() + 1));
 	memcpy(buf, p.data(), p.size());
 	buf[p.size()] = 0;
@@ -249,7 +242,6 @@ int RTLIL::OwningIdString::gc_count;
 void RTLIL::OwningIdString::collect_garbage()
 {
 	int64_t start = PerformanceTimer::query();
-#ifndef YOSYS_NO_IDS_REFCNT
 	IdStringCollector collector;
 	for (auto &[idx, design] : *RTLIL::Design::get_all_designs()) {
 		collector.trace(*design);
@@ -275,7 +267,7 @@ void RTLIL::OwningIdString::collect_garbage()
 		global_free_idx_list_.push_back(i);
 	}
 
-	for (auto it = global_autoidx_id_prefix_storage_.begin(); it != global_autoidx_id_prefix_storage_.end();) {
+	for (auto it = global_autoidx_id_storage_.begin(); it != global_autoidx_id_storage_.end();) {
 		if (collector.live.find(it->first) != collector.live.end()) {
 			++it;
 			continue;
@@ -284,14 +276,9 @@ void RTLIL::OwningIdString::collect_garbage()
 			++it;
 			continue;
 		}
-		auto str_it = global_autoidx_id_storage_.find(it->first);
-		if (str_it != global_autoidx_id_storage_.end()) {
-			delete[] str_it->second;
-			global_autoidx_id_storage_.erase(str_it);
-		}
-		it = global_autoidx_id_prefix_storage_.erase(it);
+		it = global_autoidx_id_storage_.erase(it);
 	}
-#endif
+
 	int64_t time_ns = PerformanceTimer::query() - start;
 	Pass::subtract_from_current_runtime_ns(time_ns);
 	gc_ns += time_ns;

kernel/rtlil.h

@@ -134,6 +134,17 @@ struct RTLIL::IdString
 
 		std::string_view str_view() const { return {buf, static_cast<size_t>(size)}; }
 	};
+	struct AutoidxStorage {
+		// Append the negated (i.e. positive) ID to this string to get
+		// the real string. The prefix strings must live forever.
+		const std::string *prefix;
+		// Cache of the full string, or nullptr if not cached yet.
+		std::atomic<char *> full_str;
+
+		AutoidxStorage(const std::string *prefix) : prefix(prefix), full_str(nullptr) {}
+		AutoidxStorage(AutoidxStorage&& other) : prefix(other.prefix), full_str(other.full_str.exchange(nullptr, std::memory_order_relaxed)) {}
+		~AutoidxStorage() { delete[] full_str.load(std::memory_order_acquire); }
+	};
 
 	// the global id string cache
 
@@ -147,17 +158,12 @@ struct RTLIL::IdString
 	static std::vector<Storage> global_id_storage_;
 	// Lookup table for non-autoidx IDs
 	static std::unordered_map<std::string_view, int> global_id_index_;
-	// Shared prefix string storage for autoidx IDs, which have negative
-	// indices. Append the negated (i.e. positive) ID to this string to get
-	// the real string. The prefix strings must live forever.
-	static std::unordered_map<int, const std::string*> global_autoidx_id_prefix_storage_;
-	// Explicit string storage for autoidx IDs
-	static std::unordered_map<int, char*> global_autoidx_id_storage_;
-#ifndef YOSYS_NO_IDS_REFCNT
+	// Storage for autoidx IDs, which have negative indices, i.e. all entries in this
+	// map have negative keys.
+	static std::unordered_map<int, AutoidxStorage> global_autoidx_id_storage_;
 	// All (index, refcount) pairs in this map have refcount > 0.
 	static std::unordered_map<int, int> global_refcount_storage_;
 	static std::vector<int> global_free_idx_list_;
-#endif
 
 	static int refcount(int idx) {
 		auto it = global_refcount_storage_.find(idx);
@@ -189,6 +195,7 @@ struct RTLIL::IdString
 	static int insert(std::string_view p)
 	{
 		log_assert(destruct_guard_ok);
+		log_assert(!Multithreading::active());
 
 		auto it = global_id_index_.find(p);
 		if (it != global_id_index_.end()) {
@@ -204,8 +211,9 @@ struct RTLIL::IdString
 	// Inserts an ID with string `prefix + autoidx', incrementing autoidx.
 	// `prefix` must start with '$auto$', end with '$', and live forever.
 	static IdString new_autoidx_with_prefix(const std::string *prefix) {
+		log_assert(!Multithreading::active());
 		int index = -(autoidx++);
-		global_autoidx_id_prefix_storage_.insert({index, prefix});
+		global_autoidx_id_storage_.insert({index, prefix});
 		return from_index(index);
 	}
 
@@ -238,17 +246,20 @@ struct RTLIL::IdString
 	inline const char *c_str() const {
 		if (index_ >= 0)
 			return global_id_storage_.at(index_).buf;
-		auto it = global_autoidx_id_storage_.find(index_);
-		if (it != global_autoidx_id_storage_.end())
-			return it->second;
 
-		const std::string &prefix = *global_autoidx_id_prefix_storage_.at(index_);
+		AutoidxStorage &s = global_autoidx_id_storage_.at(index_);
+		char *full_str = s.full_str.load(std::memory_order_acquire);
+		if (full_str != nullptr)
+			return full_str;
+		const std::string &prefix = *s.prefix;
 		std::string suffix = std::to_string(-index_);
 		char *c = new char[prefix.size() + suffix.size() + 1];
 		memcpy(c, prefix.data(), prefix.size());
 		memcpy(c + prefix.size(), suffix.c_str(), suffix.size() + 1);
-		global_autoidx_id_storage_.insert(it, {index_, c});
-		return c;
+		if (s.full_str.compare_exchange_strong(full_str, c, std::memory_order_acq_rel))
+			return c;
+		delete[] c;
+		return full_str;
 	}
 
 	inline std::string str() const {
@@ -262,7 +273,7 @@ struct RTLIL::IdString
 			*out += global_id_storage_.at(index_).str_view();
 			return;
 		}
-		*out += *global_autoidx_id_prefix_storage_.at(index_);
+		*out += *global_autoidx_id_storage_.at(index_).prefix;
 		*out += std::to_string(-index_);
 	}
 
@@ -348,7 +359,7 @@ struct RTLIL::IdString
 		if (index_ >= 0) {
 			return const_iterator(global_id_storage_.at(index_));
 		}
-		return const_iterator(global_autoidx_id_prefix_storage_.at(index_), -index_);
+		return const_iterator(global_autoidx_id_storage_.at(index_).prefix, -index_);
 	}
 	const_iterator end() const {
 		return const_iterator();
@@ -358,7 +369,7 @@ struct RTLIL::IdString
 		if (index_ >= 0) {
 			return Substrings(global_id_storage_.at(index_));
 		}
-		return Substrings(global_autoidx_id_prefix_storage_.at(index_), -index_);
+		return Substrings(global_autoidx_id_storage_.at(index_).prefix, -index_);
 	}
 
 	inline bool lt_by_name(const IdString &rhs) const {
@@ -411,7 +422,7 @@ struct RTLIL::IdString
 #endif
 			return *(storage.buf + i);
 		}
-		const std::string &id_start = *global_autoidx_id_prefix_storage_.at(index_);
+		const std::string &id_start = *global_autoidx_id_storage_.at(index_).prefix;
 		if (i < id_start.size())
 			return id_start[i];
 		i -= id_start.size();
@@ -597,7 +608,8 @@ private:
 	}
 	static void get_reference(int idx)
 	{
-	#ifndef YOSYS_NO_IDS_REFCNT
+		log_assert(!Multithreading::active());
+
 		if (idx < static_cast<short>(StaticId::STATIC_ID_END))
 			return;
 		auto it = global_refcount_storage_.find(idx);
@@ -605,7 +617,6 @@ private:
 			global_refcount_storage_.insert(it, {idx, 1});
 		else
 			++it->second;
-	#endif
 	#ifdef YOSYS_XTRACE_GET_PUT
 		if (yosys_xtrace && idx >= static_cast<short>(StaticId::STATIC_ID_END))
 			log("#X# GET-BY-INDEX '%s' (index %d, refcount %u)\n", from_index(idx), idx, refcount(idx));
@@ -614,7 +625,8 @@ private:
 
 	void put_reference()
 	{
-	#ifndef YOSYS_NO_IDS_REFCNT
+		log_assert(!Multithreading::active());
+
 		// put_reference() may be called from destructors after the destructor of
 		// global_refcount_storage_ has been run. in this case we simply do nothing.
 		if (index_ < static_cast<short>(StaticId::STATIC_ID_END) || !destruct_guard_ok)
@@ -628,7 +640,6 @@ private:
 		if (--it->second == 0) {
 			global_refcount_storage_.erase(it);
 		}
-	#endif
 	}
 };
 

kernel/yosys.cc

@@ -19,6 +19,7 @@
 
 #include "kernel/yosys.h"
 #include "kernel/celltypes.h"
+#include "kernel/log.h"
 
 #ifdef YOSYS_ENABLE_READLINE
 #  include <readline/readline.h>
@@ -80,7 +81,7 @@ extern "C" PyObject* PyInit_pyosys();
 
 YOSYS_NAMESPACE_BEGIN
 
-int autoidx = 1;
+Autoidx autoidx(1);
 int yosys_xtrace = 0;
 bool yosys_write_versions = true;
 const char* yosys_maybe_version() {
@@ -108,9 +109,30 @@ uint32_t Hasher::fudge = 0;
 std::string yosys_share_dirname;
 std::string yosys_abc_executable;
 
+bool Multithreading::active_ = false;
+
 void init_share_dirname();
 void init_abc_executable_name();
 
+Multithreading::Multithreading() {
+	log_assert(!active_);
+	active_ = true;
+}
+
+Multithreading::~Multithreading() {
+	log_assert(active_);
+	active_ = false;
+}
+
+void Autoidx::ensure_at_least(int v) {
+	value = std::max(value, v);
+}
+
+int Autoidx::operator++(int) {
+	log_assert(!Multithreading::active());
+	return value++;
+}
+
 void memhasher_on()
 {
 #if defined(__linux__) || defined(__FreeBSD__)

kernel/yosys_common.h

@@ -267,7 +267,30 @@ int ceil_log2(int x) YS_ATTRIBUTE(const);
 template<typename T> int GetSize(const T &obj) { return obj.size(); }
 inline int GetSize(RTLIL::Wire *wire);
 
-extern int autoidx;
+// When multiple threads are accessing RTLIL, one of these guard objects
+// must exist.
+struct Multithreading
+{
+	Multithreading();
+	~Multithreading();
+	// Returns true when multiple threads are accessing RTLIL.
+	// autoidx cannot be used during such times.
+	// IdStrings cannot be created during such times.
+	static bool active() { return active_; }
+private:
+	static bool active_;
+};
+
+struct Autoidx {
+	Autoidx(int value) : value(value) {}
+	operator int() const { return value; }
+	void ensure_at_least(int v);
+	int operator++(int);
+private:
+	int value;
+};
+
+extern Autoidx autoidx;
 extern int yosys_xtrace;
 extern bool yosys_write_versions;
 

pyosys/generator.py

@@ -164,8 +164,7 @@ pyosys_headers = [
                     {
                         "global_id_storage_",
                         "global_id_index_",
-                        "global_negative_id_storage_",
-                        "global_negative_id_prefix_storage_",
+                        "global_autoidx_id_storage_",
                         "global_refcount_storage_",
                         "global_free_idx_list_",
                         "builtin_ff_cell_types",