From 3376dcf37c02f10552f84a9602b0d05c8f77ba3a Mon Sep 17 00:00:00 2001 From: whitequark Date: Sat, 4 Apr 2020 22:53:46 +0000 Subject: [PATCH] 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). --- backends/cxxrtl/cxxrtl.cc | 103 ++++++++++++++++++++++++-------------- backends/cxxrtl/cxxrtl.h | 21 +++++--- 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 " << 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 #include +#include #include #include #include @@ -614,7 +615,6 @@ struct memory { template explicit memory(size_t depth, const init &...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 - Elem &operator [](const value &addr) { - static_assert(value::chunks <= 1, "memory indexing with unreasonably large address is not supported"); - return (*this)[addr.data[0]]; - } }; template @@ -1103,6 +1097,17 @@ value mod_ss(const value &a, const value &b) { return divmod_ss(a, b).second; } +// Memory helper +template +std::pair memory_index(const value &addr, size_t offset, size_t depth) { + static_assert(value::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 -- 2.30.2