From 38f5424f92389d6f4fdf020b214023b2b6efa71a Mon Sep 17 00:00:00 2001
From: Jim Lawson <ucbjrl@berkeley.edu>
Date: Wed, 1 May 2019 13:16:01 -0700
Subject: [PATCH] Fix #938 - Crash occurs in case when use write_firrtl command
 Add missing memory initialization. Sanity-check memory parameters. Add Cell
 pointer to memory object (for error reporting).

---
 backends/firrtl/firrtl.cc   | 45 +++++++++++++++++++++++++++++++++----
 tests/memories/firrtl_938.v | 22 ++++++++++++++++++
 tests/simple/xfirrtl        |  1 +
 3 files changed, 64 insertions(+), 4 deletions(-)
 create mode 100644 tests/memories/firrtl_938.v

diff --git a/backends/firrtl/firrtl.cc b/backends/firrtl/firrtl.cc
index ed6e9f8ee..a8a1bb078 100644
--- a/backends/firrtl/firrtl.cc
+++ b/backends/firrtl/firrtl.cc
@@ -164,6 +164,7 @@ struct FirrtlWorker
 	};
 	/* Memories defined within this module. */
 	 struct memory {
+		 Cell *pCell;					// for error reporting
 		 string name;					// memory name
 		 int abits;						// number of address bits
 		 int size;						// size (in units) of the memory
@@ -174,8 +175,37 @@ struct FirrtlWorker
 		 vector<write_port> write_ports;
 		 std::string init_file;
 		 std::string init_file_srcFileSpec;
-		 memory(string name, int abits, int size, int width) : name(name), abits(abits), size(size), width(width), read_latency(0), write_latency(1), init_file(""), init_file_srcFileSpec("") {}
-		 memory() : read_latency(0), write_latency(1), init_file(""), init_file_srcFileSpec(""){}
+		 string srcLine;
+		 memory(Cell *pCell, string name, int abits, int size, int width) : pCell(pCell), name(name), abits(abits), size(size), width(width), read_latency(0), write_latency(1), init_file(""), init_file_srcFileSpec("") {
+			// Provide defaults for abits or size if one (but not the other) is specified.
+			if (this->abits == 0 && this->size != 0) {
+				this->abits = ceil_log2(this->size);
+			} else if (this->abits != 0 && this->size == 0) {
+				this->size = 1 << this->abits;
+			}
+			// Sanity-check this construction.
+			if (this->name == "") {
+				log_error("Nameless memory%s\n", this->atLine());
+			}
+			if (this->abits == 0 && this->size == 0) {
+				log_error("Memory %s has zero address bits and size%s\n", this->name.c_str(), this->atLine());
+			}
+			if (this->width == 0) {
+				log_error("Memory %s has zero width%s\n", this->name.c_str(), this->atLine());
+			}
+		 }
+		// We need a default constructor for the dict insert.
+	   memory() : pCell(0), read_latency(0), write_latency(1), init_file(""), init_file_srcFileSpec(""){}
+
+		const char *atLine() {
+			if (srcLine == "") {
+				if (pCell) {
+					auto p = pCell->attributes.find("\\src");
+					srcLine = " at " + p->second.decode_string();
+				}
+			}
+			return srcLine.c_str();
+		}
 		 void add_memory_read_port(read_port &rp) {
 			 read_ports.push_back(rp);
 		 }
@@ -604,7 +634,7 @@ struct FirrtlWorker
 				int abits = cell->parameters.at("\\ABITS").as_int();
 				int width = cell->parameters.at("\\WIDTH").as_int();
 				int size = cell->parameters.at("\\SIZE").as_int();
-				memory m(mem_id, abits, size, width);
+				memory m(cell, mem_id, abits, size, width);
 				int rd_ports = cell->parameters.at("\\RD_PORTS").as_int();
 				int wr_ports = cell->parameters.at("\\WR_PORTS").as_int();
 
@@ -681,6 +711,8 @@ struct FirrtlWorker
 			{
 				std::string cell_type = fid(cell->type);
 				std::string mem_id = make_id(cell->parameters["\\MEMID"].decode_string());
+				int abits = cell->parameters.at("\\ABITS").as_int();
+				int width = cell->parameters.at("\\WIDTH").as_int();
 				memory *mp = nullptr;
 				if (cell->type == "$meminit" ) {
 					log_error("$meminit (%s.%s.%s) currently unsupported\n", log_id(module), log_id(cell), mem_id.c_str());
@@ -693,6 +725,11 @@ struct FirrtlWorker
 					Const clk_enable = cell->parameters.at("\\CLK_ENABLE");
 					Const clk_polarity = cell->parameters.at("\\CLK_POLARITY");
 
+					// Do we already have an entry for this memory?
+					if (memories.count(mem_id) == 0) {
+						memory m(cell, mem_id, abits, 0, width);
+						register_memory(m);
+					}
 					mp = &memories.at(mem_id);
 					int portNum = 0;
 					bool transparency = false;
@@ -890,7 +927,7 @@ struct FirrtlWorker
 
 		// If we have any memory definitions, output them.
 		for (auto kv : memories) {
-			memory m = kv.second;
+			memory &m = kv.second;
 			f << stringf("    mem %s:\n", m.name.c_str());
 			f << stringf("      data-type => UInt<%d>\n", m.width);
 			f << stringf("      depth => %d\n", m.size);
diff --git a/tests/memories/firrtl_938.v b/tests/memories/firrtl_938.v
new file mode 100644
index 000000000..af5efcd25
--- /dev/null
+++ b/tests/memories/firrtl_938.v
@@ -0,0 +1,22 @@
+module top
+(
+	input [7:0] data_a,
+	input [6:1] addr_a,
+	input we_a, clk,
+	output reg [7:0] q_a
+);
+	// Declare the RAM variable
+	reg [7:0] ram[63:0];
+
+	// Port A
+	always @ (posedge clk)
+	begin
+		if (we_a)
+		begin
+			ram[addr_a] <= data_a;
+			q_a <= data_a;
+		end
+			q_a <= ram[addr_a];
+	end
+
+endmodule
diff --git a/tests/simple/xfirrtl b/tests/simple/xfirrtl
index 50d693513..ba61a4476 100644
--- a/tests/simple/xfirrtl
+++ b/tests/simple/xfirrtl
@@ -16,6 +16,7 @@ operators.v	$pow
 partsel.v	drops modules
 process.v	drops modules
 realexpr.v	drops modules
+retime.v	Initial value (11110101) for (retime_test.ff) not supported
 scopes.v	original verilog issues ( -x where x isn't declared signed)
 sincos.v	$adff
 specify.v	no code (empty module generates error