Merge pull request #3682 from daglem/struct-member-out-of-bounds

Out of bounds checking for struct/union members

8 files changed, 145 insertions, 22 deletions

diff --git a/frontends/ast/ast.cc b/frontends/ast/ast.cc
index 982943d1b..5a2ade04a 100644
--- a/frontends/ast/ast.cc
+++ b/frontends/ast/ast.cc
@@ -524,7 +524,13 @@ void AstNode::dumpVlog(FILE *f, std::string indent) const
 		break;
 
 	case AST_IDENTIFIER:
-		fprintf(f, "%s", id2vl(str).c_str());
+		{
+			AST::AstNode *member_node = AST::get_struct_member(this);
+			if (member_node)
+				fprintf(f, "%s[%d:%d]", id2vl(str).c_str(), member_node->range_left, member_node->range_right);
+			else
+				fprintf(f, "%s", id2vl(str).c_str());
+		}
 		for (auto child : children)
 			child->dumpVlog(f, "");
 		break;
diff --git a/frontends/ast/ast.h b/frontends/ast/ast.h
index 142ec0801..25a600f00 100644
--- a/frontends/ast/ast.h
+++ b/frontends/ast/ast.h
@@ -380,6 +380,7 @@ namespace AST
 
 	// struct helper exposed from simplify for genrtlil
 	AstNode *make_struct_member_range(AstNode *node, AstNode *member_node);
+	AstNode *get_struct_member(const AstNode *node);
 
 	// generate standard $paramod... derived module name; parameters should be
 	// in the order they are declared in the instantiated module
diff --git a/frontends/ast/genrtlil.cc b/frontends/ast/genrtlil.cc
index 1016ef636..8da4b0b0a 100644
--- a/frontends/ast/genrtlil.cc
+++ b/frontends/ast/genrtlil.cc
@@ -1375,6 +1375,7 @@ RTLIL::SigSpec AstNode::genRTLIL(int width_hint, bool sign_hint)
 			RTLIL::SigChunk chunk;
 			bool is_interface = false;
 
+			AST::AstNode *member_node = NULL;
 			int add_undef_bits_msb = 0;
 			int add_undef_bits_lsb = 0;
 
@@ -1438,12 +1439,28 @@ RTLIL::SigSpec AstNode::genRTLIL(int width_hint, bool sign_hint)
 			chunk.width = wire->width;
 			chunk.offset = 0;
 
+			if ((member_node = get_struct_member(this))) {
+				// Clamp wire chunk to range of member within struct/union.
+				chunk.width = member_node->range_left - member_node->range_right + 1;
+				chunk.offset = member_node->range_right;
+			}
+
 		use_const_chunk:
 			if (children.size() != 0) {
 				if (children[0]->type != AST_RANGE)
 					log_file_error(filename, location.first_line, "Single range expected.\n");
 				int source_width = id2ast->range_left - id2ast->range_right + 1;
 				int source_offset = id2ast->range_right;
+				int chunk_left = source_width - 1;
+				int chunk_right = 0;
+
+				if (member_node) {
+					// Clamp wire chunk to range of member within struct/union.
+					log_assert(!source_offset && !id2ast->range_swapped);
+					chunk_left = chunk.offset + chunk.width - 1;
+					chunk_right = chunk.offset;
+				}
+
 				if (!children[0]->range_valid) {
 					AstNode *left_at_zero_ast = children[0]->children[0]->clone();
 					AstNode *right_at_zero_ast = children[0]->children.size() >= 2 ? children[0]->children[1]->clone() : left_at_zero_ast->clone();
@@ -1455,14 +1472,16 @@ RTLIL::SigSpec AstNode::genRTLIL(int width_hint, bool sign_hint)
 					AstNode *fake_ast = new AstNode(AST_NONE, clone(), children[0]->children.size() >= 2 ?
 							children[0]->children[1]->clone() : children[0]->children[0]->clone());
 					fake_ast->children[0]->delete_children();
+					if (member_node)
+						fake_ast->children[0]->attributes[ID::wiretype] = member_node->clone();
 
 					int fake_ast_width = 0;
 					bool fake_ast_sign = true;
 					fake_ast->children[1]->detectSignWidth(fake_ast_width, fake_ast_sign);
 					RTLIL::SigSpec shift_val = fake_ast->children[1]->genRTLIL(fake_ast_width, fake_ast_sign);
 
-					if (id2ast->range_right != 0) {
-						shift_val = current_module->Sub(NEW_ID, shift_val, id2ast->range_right, fake_ast_sign);
+					if (source_offset != 0) {
+						shift_val = current_module->Sub(NEW_ID, shift_val, source_offset, fake_ast_sign);
 						fake_ast->children[1]->is_signed = true;
 					}
 					if (id2ast->range_swapped) {
@@ -1478,10 +1497,10 @@ RTLIL::SigSpec AstNode::genRTLIL(int width_hint, bool sign_hint)
 					return sig;
 				} else {
 					chunk.width = children[0]->range_left - children[0]->range_right + 1;
-					chunk.offset = children[0]->range_right - source_offset;
+					chunk.offset += children[0]->range_right - source_offset;
 					if (id2ast->range_swapped)
-						chunk.offset = (id2ast->range_left - id2ast->range_right + 1) - (chunk.offset + chunk.width);
-					if (chunk.offset >= source_width || chunk.offset + chunk.width < 0) {
+						chunk.offset = source_width - (chunk.offset + chunk.width);
+					if (chunk.offset > chunk_left || chunk.offset + chunk.width < chunk_right) {
 						if (chunk.width == 1)
 							log_file_warning(filename, location.first_line, "Range select out of bounds on signal `%s': Setting result bit to undef.\n",
 									str.c_str());
@@ -1490,12 +1509,12 @@ RTLIL::SigSpec AstNode::genRTLIL(int width_hint, bool sign_hint)
 									children[0]->range_left, children[0]->range_right, str.c_str(), chunk.width);
 						chunk = RTLIL::SigChunk(RTLIL::State::Sx, chunk.width);
 					} else {
-						if (chunk.width + chunk.offset > source_width) {
-							add_undef_bits_msb = (chunk.width + chunk.offset) - source_width;
+						if (chunk.offset + chunk.width - 1 > chunk_left) {
+							add_undef_bits_msb = (chunk.offset + chunk.width - 1) - chunk_left;
 							chunk.width -= add_undef_bits_msb;
 						}
-						if (chunk.offset < 0) {
-							add_undef_bits_lsb = -chunk.offset;
+						if (chunk.offset < chunk_right) {
+							add_undef_bits_lsb = chunk_right - chunk.offset;
 							chunk.width -= add_undef_bits_lsb;
 							chunk.offset += add_undef_bits_lsb;
 						}
diff --git a/frontends/ast/simplify.cc b/frontends/ast/simplify.cc
index 7edff38d9..dfb1b890c 100644
--- a/frontends/ast/simplify.cc
+++ b/frontends/ast/simplify.cc
@@ -445,7 +445,7 @@ static AstNode *struct_index_lsb_offset(AstNode *lsb_offset, AstNode *rnode, Ast
 	stride /= get_struct_range_width(member_node, dimension);
 	auto right = normalize_struct_index(rnode->children.back(), member_node, dimension);
 	auto offset = stride > 1 ? multiply_by_const(right, stride) : right;
-	return new AstNode(AST_ADD, lsb_offset, offset);
+	return lsb_offset ? new AstNode(AST_ADD, lsb_offset, offset) : offset;
 }
 
 static AstNode *struct_index_msb_offset(AstNode *lsb_offset, AstNode *rnode, AstNode *member_node, int dimension, int stride)
@@ -484,7 +484,7 @@ AstNode *AST::make_struct_member_range(AstNode *node, AstNode *member_node)
 	int range_right = member_node->range_right;
 	if (node->children.empty()) {
 		// no range operations apply, return the whole width
-		return make_range(range_left, range_right);
+		return make_range(range_left - range_right, 0);
 	}
 
 	if (node->children.size() != 1) {
@@ -493,7 +493,7 @@ AstNode *AST::make_struct_member_range(AstNode *node, AstNode *member_node)
 
 	// Range operations
 	auto rnode = node->children[0];
-	auto lsb_offset = node_int(member_node->range_right);
+	AstNode *lsb_offset = NULL;
 	int stride = range_left - range_right + 1;
 	size_t i = 0;
 
@@ -520,6 +520,17 @@ AstNode *AST::make_struct_member_range(AstNode *node, AstNode *member_node)
 	return new AstNode(AST_RANGE, msb_offset, lsb_offset);
 }
 
+AstNode *AST::get_struct_member(const AstNode *node)
+{
+	AST::AstNode *member_node;
+	if (node->attributes.count(ID::wiretype) && (member_node = node->attributes.at(ID::wiretype)) &&
+	    (member_node->type == AST_STRUCT_ITEM || member_node->type == AST_STRUCT || member_node->type == AST_UNION))
+	{
+		return member_node;
+	}
+	return NULL;
+}
+
 static void add_members_to_scope(AstNode *snode, std::string name)
 {
 	// add all the members in a struct or union to local scope
@@ -2696,7 +2707,14 @@ bool AstNode::simplify(bool const_fold, bool at_zero, bool in_lvalue, int stage,
 			goto skip_dynamic_range_lvalue_expansion;
 
 		int source_width = children[0]->id2ast->range_left - children[0]->id2ast->range_right + 1;
+		int source_offset = children[0]->id2ast->range_right;
 		int result_width = 1;
+		AST::AstNode *member_node = get_struct_member(children[0]);
+		if (member_node) {
+			// Clamp chunk to range of member within struct/union.
+			log_assert(!source_offset && !children[0]->id2ast->range_swapped);
+			source_width = member_node->range_left - member_node->range_right + 1;
+		}
 
 		AstNode *shift_expr = NULL;
 		AstNode *range = children[0]->children[0];
@@ -2737,11 +2755,13 @@ bool AstNode::simplify(bool const_fold, bool at_zero, bool in_lvalue, int stage,
 			did_something = true;
 			newNode = new AstNode(AST_CASE, shift_expr);
 			for (int i = 0; i < source_width; i++) {
-				int start_bit = children[0]->id2ast->range_right + i;
+				int start_bit = source_offset + i;
 				int end_bit = std::min(start_bit+result_width,source_width) - 1;
 				AstNode *cond = new AstNode(AST_COND, mkconst_int(start_bit, true));
 				AstNode *lvalue = children[0]->clone();
 				lvalue->delete_children();
+				if (member_node)
+					lvalue->attributes[ID::wiretype] = member_node->clone();
 				lvalue->children.push_back(new AstNode(AST_RANGE,
 						mkconst_int(end_bit, true), mkconst_int(start_bit, true)));
 				cond->children.push_back(new AstNode(AST_BLOCK, new AstNode(type, lvalue, children[1]->clone())));
@@ -2783,6 +2803,8 @@ bool AstNode::simplify(bool const_fold, bool at_zero, bool in_lvalue, int stage,
 
 			AstNode *lvalue = children[0]->clone();
 			lvalue->delete_children();
+			if (member_node)
+				lvalue->attributes[ID::wiretype] = member_node->clone();
 
 			AstNode *ref_mask = new AstNode(AST_IDENTIFIER);
 			ref_mask->str = wire_mask->str;
@@ -2813,7 +2835,7 @@ bool AstNode::simplify(bool const_fold, bool at_zero, bool in_lvalue, int stage,
 			shamt = new AstNode(AST_TO_SIGNED, shamt);
 
 			// offset the shift amount by the lower bound of the dimension
-			int start_bit = children[0]->id2ast->range_right;
+			int start_bit = source_offset;
 			shamt = new AstNode(AST_SUB, shamt, mkconst_int(start_bit, true));
 
 			// reflect the shift amount if the dimension is swapped
@@ -3403,11 +3425,8 @@ skip_dynamic_range_lvalue_expansion:;
 						log_file_error(filename, location.first_line, "Failed to resolve identifier %s for width detection!\n", buf->str.c_str());
 
 					// Check for item in packed struct / union
-					AST::AstNode *item_node;
-					if (id_ast->type == AST_WIRE &&
-					    buf->attributes.count(ID::wiretype) && (item_node = buf->attributes[ID::wiretype]) &&
-					    (item_node->type == AST_STRUCT_ITEM || item_node->type == AST_STRUCT || item_node->type == AST_UNION))
-					{
+					AST::AstNode *item_node = get_struct_member(buf);
+					if (id_ast->type == AST_WIRE && item_node) {
 						// The dimension of the original array expression is saved in the 'integer' field
 						dim += buf->integer;
 						if (item_node->multirange_dimensions.empty()) {
@@ -4082,10 +4101,13 @@ replace_fcall_later:;
 							tmp_range_left = (param_width + 2*param_offset) - children[0]->range_right - 1;
 							tmp_range_right = (param_width + 2*param_offset) - children[0]->range_left - 1;
 						}
+						AST::AstNode *member_node = get_struct_member(this);
+						int chunk_offset = member_node ? member_node->range_right : 0;
+						log_assert(!(chunk_offset && param_upto));
 						for (int i = tmp_range_right; i <= tmp_range_left; i++) {
 							int index = i - param_offset;
 							if (0 <= index && index < param_width)
-								data.push_back(current_scope[str]->children[0]->bits[index]);
+								data.push_back(current_scope[str]->children[0]->bits[chunk_offset + index]);
 							else
 								data.push_back(RTLIL::State::Sx);
 						}
diff --git a/tests/gen-tests-makefile.sh b/tests/gen-tests-makefile.sh
index cde9ab1b9..3df36a963 100755
--- a/tests/gen-tests-makefile.sh
+++ b/tests/gen-tests-makefile.sh
@@ -75,7 +75,7 @@ generate_tests() {
 	if [[ $do_sv = true ]]; then
 		for x in *.sv; do
 			if [ ! -f "${x%.sv}.ys"  ]; then
-				generate_ys_test "$x" "-p \"prep -top top; sat -verify -prove-asserts\" $yosys_args"
+				generate_ys_test "$x" "-p \"prep -top top; sat -enable_undef -verify -prove-asserts\" $yosys_args"
 			fi;
 		done
 	fi;
diff --git a/tests/svtypes/struct_array.sv b/tests/svtypes/struct_array.sv
index a0b84640d..bedc05b6f 100644
--- a/tests/svtypes/struct_array.sv
+++ b/tests/svtypes/struct_array.sv
@@ -12,12 +12,16 @@ module top;
 
 		s.a[2:1] = 16'h1234;
 		s.a[5] = 8'h42;
+		s.a[-1] = '0;
 
 		s.b = '1;
 		s.b[1:0] = '0;
 	end
 
 	always_comb assert(s==64'h4200_0012_3400_FFFC);
+	always_comb assert(s.a[0][3:-4]===8'h0x);
+	always_comb assert(s.b[23:16]===8'hxx);
+	always_comb assert(s.b[19:12]===8'hxf);
 
 	struct packed {
 		bit [7:0] [7:0] a;	// 8 element packed array of bytes
diff --git a/tests/svtypes/struct_dynamic_range.sv b/tests/svtypes/struct_dynamic_range.sv
new file mode 100644
index 000000000..ce1f14670
--- /dev/null
+++ b/tests/svtypes/struct_dynamic_range.sv
@@ -0,0 +1,67 @@
+module range_shift_mask(
+    input logic [2:0] addr_i,
+    input logic [7:0] data_i,
+    input logic [2:0] addr_o,
+    output logic [7:0] data_o
+);
+    // (* nowrshmsk = 0 *)
+    struct packed {
+        logic [7:0] msb;
+        logic [0:3][7:0] data;
+        logic [7:0] lsb;
+    } s;
+
+    always_comb begin
+        s = '1;
+        s.data[addr_i] = data_i;
+        data_o = s.data[addr_o];
+    end
+endmodule
+
+module range_case(
+    input logic [2:0] addr_i,
+    input logic [7:0] data_i,
+    input logic [2:0] addr_o,
+    output logic [7:0] data_o
+);
+    // (* nowrshmsk = 1 *)
+    struct packed {
+        logic [7:0] msb;
+        logic [0:3][7:0] data;
+        logic [7:0] lsb;
+    } s;
+
+    always_comb begin
+        s = '1;
+        s.data[addr_i] = data_i;
+        data_o = s.data[addr_o];
+    end
+endmodule
+
+module top;
+    logic [7:0] data_shift_mask1;
+    range_shift_mask range_shift_mask1(3'd1, 8'h7e, 3'd1, data_shift_mask1);
+    logic [7:0] data_shift_mask2;
+    range_shift_mask range_shift_mask2(3'd1, 8'h7e, 3'd2, data_shift_mask2);
+    logic [7:0] data_shift_mask3;
+    range_shift_mask range_shift_mask3(3'd1, 8'h7e, 3'd4, data_shift_mask3);
+
+    always_comb begin
+        assert(data_shift_mask1 === 8'h7e);
+        assert(data_shift_mask2 === 8'hff);
+        assert(data_shift_mask3 === 8'hxx);
+    end
+
+    logic [7:0] data_case1;
+    range_case range_case1(3'd1, 8'h7e, 3'd1, data_case1);
+    logic [7:0] data_case2;
+    range_case range_case2(3'd1, 8'h7e, 3'd2, data_case2);
+    logic [7:0] data_case3;
+    range_case range_case3(3'd1, 8'h7e, 3'd4, data_case3);
+
+    always_comb begin
+        assert(data_case1 === 8'h7e);
+        assert(data_case2 === 8'hff);
+        assert(data_case3 === 8'hxx);
+    end
+endmodule
diff --git a/tests/svtypes/struct_dynamic_range.ys b/tests/svtypes/struct_dynamic_range.ys
new file mode 100644
index 000000000..d09e1924d
--- /dev/null
+++ b/tests/svtypes/struct_dynamic_range.ys
@@ -0,0 +1,4 @@
+read_verilog -sv struct_dynamic_range.sv
+prep -top top
+flatten
+sat -enable_undef -verify -prove-asserts