write_cxxrtl: avoid undefined behavior on out-of-bounds memory access.

After this commit, if NDEBUG is not defined, out-of-bounds accesses
cause assertion failures for reads and writes. If NDEBUG is defined,
out-of-bounds reads return zeroes, and out-of-bounds writes are
ignored.

This commit also adds support for memories that start with a non-zero
index (`Memory::start_offset` in RTLIL).

2 files changed, 78 insertions, 46 deletions

diff --git a/backends/cxxrtl/cxxrtl.cc b/backends/cxxrtl/cxxrtl.cc
index 94da61a2c..8a9e8348b 100644
--- a/backends/cxxrtl/cxxrtl.cc
+++ b/backends/cxxrtl/cxxrtl.cc
@@ -798,6 +798,10 @@ struct CxxrtlWorker {
 				inc_indent();
 			}
 			RTLIL::Memory *memory = cell->module->memories[cell->getParam(ID(MEMID)).decode_string()];
+			std::string valid_index_temp = fresh_temporary();
+			f << indent << "std::pair<bool, size_t> " << valid_index_temp << " = memory_index(";
+			dump_sigspec_rhs(cell->getPort(ID(ADDR)));
+			f << ", " << memory->start_offset << ", " << memory->size << ");\n";
 			if (cell->type == ID($memrd)) {
 				if (!cell->getPort(ID(EN)).is_fully_ones()) {
 					f << indent << "if (";
@@ -805,38 +809,54 @@ struct CxxrtlWorker {
 					f << ") {\n";
 					inc_indent();
 				}
-				if (writable_memories[memory]) {
-					std::string addr_temp = fresh_temporary();
-					f << indent << "const value<" << cell->getPort(ID(ADDR)).size() << "> &" << addr_temp << " = ";
-					dump_sigspec_rhs(cell->getPort(ID(ADDR)));
-					f << ";\n";
-					std::string lhs_temp = fresh_temporary();
-					f << indent << "value<" << memory->width << "> " << lhs_temp << " = "
-					            << mangle(memory) << "[" << addr_temp << "].curr;\n";
-					for (auto memwr_cell : transparent_for[cell]) {
-						f << indent << "if (" << addr_temp << " == ";
-						dump_sigspec_rhs(memwr_cell->getPort(ID(ADDR)));
-						f << ") {\n";
-						inc_indent();
-							f << indent << lhs_temp << " = " << lhs_temp;
-							f << ".update(";
-							dump_sigspec_rhs(memwr_cell->getPort(ID(EN)));
-							f << ", ";
-							dump_sigspec_rhs(memwr_cell->getPort(ID(DATA)));
-							f << ");\n";
-						dec_indent();
-						f << indent << "}\n";
+				// The generated code has two bounds checks; one in an assertion, and another that guards the read.
+				// This is done so that the code does not invoke undefined behavior under any conditions, but nevertheless
+				// loudly crashes if an illegal condition is encountered. The assert may be turned off with -NDEBUG not
+				// just for release builds, but also to make sure the simulator (which is presumably embedded in some
+				// larger program) will never crash the code that calls into it.
+				//
+				// If assertions are disabled, out of bounds reads are defined to return zero.
+				f << indent << "assert(" << valid_index_temp << ".first && \"out of bounds read\");\n";
+				f << indent << "if(" << valid_index_temp << ".first) {\n";
+				inc_indent();
+					if (writable_memories[memory]) {
+						std::string addr_temp = fresh_temporary();
+						f << indent << "const value<" << cell->getPort(ID(ADDR)).size() << "> &" << addr_temp << " = ";
+						dump_sigspec_rhs(cell->getPort(ID(ADDR)));
+						f << ";\n";
+						std::string lhs_temp = fresh_temporary();
+						f << indent << "value<" << memory->width << "> " << lhs_temp << " = "
+						            << mangle(memory) << "[" << valid_index_temp << ".second].curr;\n";
+						for (auto memwr_cell : transparent_for[cell]) {
+							f << indent << "if (" << addr_temp << " == ";
+							dump_sigspec_rhs(memwr_cell->getPort(ID(ADDR)));
+							f << ") {\n";
+							inc_indent();
+								f << indent << lhs_temp << " = " << lhs_temp;
+								f << ".update(";
+								dump_sigspec_rhs(memwr_cell->getPort(ID(EN)));
+								f << ", ";
+								dump_sigspec_rhs(memwr_cell->getPort(ID(DATA)));
+								f << ");\n";
+							dec_indent();
+							f << indent << "}\n";
+						}
+						f << indent;
+						dump_sigspec_lhs(cell->getPort(ID(DATA)));
+						f << " = " << lhs_temp << ";\n";
+					} else {
+						f << indent;
+						dump_sigspec_lhs(cell->getPort(ID(DATA)));
+						f << " = " << mangle(memory) << "[" << valid_index_temp << ".second];\n";
 					}
+				dec_indent();
+				f << indent << "} else {\n";
+				inc_indent();
 					f << indent;
 					dump_sigspec_lhs(cell->getPort(ID(DATA)));
-					f << " = " << lhs_temp << ";\n";
-				} else {
-					f << indent;
-					dump_sigspec_lhs(cell->getPort(ID(DATA)));
-					f << " = " << mangle(memory) << "[";
-					dump_sigspec_rhs(cell->getPort(ID(ADDR)));
-					f << "];\n";
-				}
+					f << " = value<" << memory->width << "> {};\n";
+				dec_indent();
+				f << indent << "}\n";
 				if (!cell->getPort(ID(EN)).is_fully_ones()) {
 					dec_indent();
 					f << indent << "}\n";
@@ -844,15 +864,22 @@ struct CxxrtlWorker {
 			} else /*if (cell->type == ID($memwr))*/ {
 				// FIXME: handle write port priority, here and above in transparent $memrd cells
 				log_assert(writable_memories[memory]);
-				std::string lhs_temp = fresh_temporary();
-				f << indent << "wire<" << memory->width << "> &" << lhs_temp << " = " << mangle(memory) << "[";
-				dump_sigspec_rhs(cell->getPort(ID(ADDR)));
-				f << "];\n";
-				f << indent << lhs_temp << ".next = " << lhs_temp << ".curr.update(";
-				dump_sigspec_rhs(cell->getPort(ID(EN)));
-				f << ", ";
-				dump_sigspec_rhs(cell->getPort(ID(DATA)));
-				f << ");\n";
+				// See above for rationale of having both the assert and the condition.
+				//
+				// If assertions are disabled, out of bounds writes are defined to do nothing.
+				f << indent << "assert(" << valid_index_temp << ".first && \"out of bounds write\");\n";
+				f << indent << "if (" << valid_index_temp << ".first) {\n";
+				inc_indent();
+					std::string lhs_temp = fresh_temporary();
+					f << indent << "wire<" << memory->width << "> &" << lhs_temp << " = ";
+					f << mangle(memory) << "[" << valid_index_temp << ".second];\n";
+					f << indent << lhs_temp << ".next = " << lhs_temp << ".curr.update(";
+					dump_sigspec_rhs(cell->getPort(ID(EN)));
+					f << ", ";
+					dump_sigspec_rhs(cell->getPort(ID(DATA)));
+					f << ");\n";
+				dec_indent();
+				f << indent << "}\n";
 			}
 			if (cell->getParam(ID(CLK_ENABLE)).as_bool()) {
 				dec_indent();
diff --git a/backends/cxxrtl/cxxrtl.h b/backends/cxxrtl/cxxrtl.h
index a67591885..18e45e22c 100644
--- a/backends/cxxrtl/cxxrtl.h
+++ b/backends/cxxrtl/cxxrtl.h
@@ -23,6 +23,7 @@
 
 #include <cstddef>
 #include <cstdint>
+#include <cassert>
 #include <limits>
 #include <type_traits>
 #include <tuple>
@@ -614,7 +615,6 @@ struct memory {
 
 	template<size_t... InitSize>
 	explicit memory(size_t depth, const init<InitSize> &...init) : data(depth) {
-		// FIXME: assert(init.size() <= depth);
 		data.resize(depth);
 		// This utterly reprehensible construct is the most reasonable way to apply a function to every element
 		// of a parameter pack, if the elements all have different types and so cannot be cast to an initializer list.
@@ -622,15 +622,9 @@ struct memory {
 	}
 
 	Elem &operator [](size_t index) {
-		// FIXME: assert(index < data.size());
+		assert(index < data.size());
 		return data[index];
 	}
-
-	template<size_t AddrBits>
-	Elem &operator [](const value<AddrBits> &addr) {
-		static_assert(value<AddrBits>::chunks <= 1, "memory indexing with unreasonably large address is not supported");
-		return (*this)[addr.data[0]];
-	}
 };
 
 template<size_t Width>
@@ -1103,6 +1097,17 @@ value<BitsY> mod_ss(const value<BitsA> &a, const value<BitsB> &b) {
 	return divmod_ss<BitsY>(a, b).second;
 }
 
+// Memory helper
+template<size_t BitsAddr>
+std::pair<bool, size_t> memory_index(const value<BitsAddr> &addr, size_t offset, size_t depth) {
+	static_assert(value<BitsAddr>::chunks <= 1, "memory address is too wide");
+	size_t offset_index = addr.data[0];
+
+	bool valid = (offset_index >= offset && offset_index < offset + depth);
+	size_t index = offset_index - offset;
+	return std::make_pair(valid, index);
+}
+
 } // namespace cxxrtl_yosys
 
 #endif