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).

diff --git a/backends/cxxrtl/cxxrtl.cc b/backends/cxxrtl/cxxrtl.cc
index 94da61a2c8a1c1a7d9f6fe8fe3675ce29241b2be..8a9e8348b61f87ce4d956bfb72687b82b64a2688 100644 (file)
--- 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 a67591885893f915ced3276ecf5b57cbfd154fdc..18e45e22c35b6488aff87a81032e09134cc35817 100644 (file)
--- 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