From 79043cb849e01b494e1ab432dc52f5f99d5ff4af Mon Sep 17 00:00:00 2001 From: Dag Lem Date: Sun, 19 Feb 2023 23:25:08 +0100 Subject: Out of bounds checking for struct/union members Currently, only constant indices are checked. --- frontends/ast/genrtlil.cc | 23 ++++++++++++++++++----- tests/gen-tests-makefile.sh | 2 +- tests/svtypes/struct_array.sv | 3 +++ 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/frontends/ast/genrtlil.cc b/frontends/ast/genrtlil.cc index 1016ef636..9f458530d 100644 --- a/frontends/ast/genrtlil.cc +++ b/frontends/ast/genrtlil.cc @@ -1444,6 +1444,19 @@ RTLIL::SigSpec AstNode::genRTLIL(int width_hint, bool sign_hint) 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 item_left = source_width - 1; + int item_right = 0; + + // Check for item in struct/union. + AST::AstNode *item_node; + if (attributes.count(ID::wiretype) && (item_node = attributes[ID::wiretype]) && + (item_node->type == AST_STRUCT_ITEM || item_node->type == AST_STRUCT || item_node->type == AST_UNION)) + { + // Clamp chunk to range of item within struct/union. + item_left = item_node->range_left; + item_right = item_node->range_right; + } + 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(); @@ -1481,7 +1494,7 @@ RTLIL::SigSpec AstNode::genRTLIL(int width_hint, bool sign_hint) 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) { + if (chunk.offset > item_left || chunk.offset + chunk.width < item_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 +1503,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 > item_left) { + add_undef_bits_msb = (chunk.offset + chunk.width - 1) - item_left; chunk.width -= add_undef_bits_msb; } - if (chunk.offset < 0) { - add_undef_bits_lsb = -chunk.offset; + if (chunk.offset < item_right) { + add_undef_bits_lsb = item_right - chunk.offset; chunk.width -= add_undef_bits_lsb; chunk.offset += add_undef_bits_lsb; } 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..b87f936aa 100644 --- a/tests/svtypes/struct_array.sv +++ b/tests/svtypes/struct_array.sv @@ -18,6 +18,9 @@ module top; end always_comb assert(s==64'h4200_0012_3400_FFFC); + always_comb assert(s.b[23:16]===8'hxx); + always_comb assert(s.b[19:12]===8'hxf); + always_comb assert(s.a[0][3:-4]===8'h0x); struct packed { bit [7:0] [7:0] a; // 8 element packed array of bytes -- cgit v1.2.3 From 0d3423ddea1c24aea74206d64e6dc5196959ad5e Mon Sep 17 00:00:00 2001 From: Dag Lem Date: Tue, 28 Feb 2023 18:45:55 +0100 Subject: Index struct/union members within corresponding wire chunks This guards against access to bits outside of struct/union members via dynamic indexing. --- frontends/ast/ast.cc | 8 +++++++- frontends/ast/ast.h | 1 + frontends/ast/genrtlil.cc | 46 ++++++++++++++++++++++++------------------- frontends/ast/simplify.cc | 44 ++++++++++++++++++++++++++++++----------- tests/svtypes/struct_array.sv | 3 ++- 5 files changed, 69 insertions(+), 33 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 9f458530d..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,23 +1439,26 @@ 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 item_left = source_width - 1; - int item_right = 0; - - // Check for item in struct/union. - AST::AstNode *item_node; - if (attributes.count(ID::wiretype) && (item_node = attributes[ID::wiretype]) && - (item_node->type == AST_STRUCT_ITEM || item_node->type == AST_STRUCT || item_node->type == AST_UNION)) - { - // Clamp chunk to range of item within struct/union. - item_left = item_node->range_left; - item_right = item_node->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) { @@ -1468,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) { @@ -1491,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 > item_left || chunk.offset + chunk.width < item_right) { + 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()); @@ -1503,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.offset + chunk.width - 1 > item_left) { - add_undef_bits_msb = (chunk.offset + chunk.width - 1) - item_left; + 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 < item_right) { - add_undef_bits_lsb = item_right - 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/svtypes/struct_array.sv b/tests/svtypes/struct_array.sv index b87f936aa..bedc05b6f 100644 --- a/tests/svtypes/struct_array.sv +++ b/tests/svtypes/struct_array.sv @@ -12,15 +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); - always_comb assert(s.a[0][3:-4]===8'h0x); struct packed { bit [7:0] [7:0] a; // 8 element packed array of bytes -- cgit v1.2.3 From 1af7d6121f697b60e6eaabcabd50c49c90d09402 Mon Sep 17 00:00:00 2001 From: Dag Lem Date: Wed, 8 Mar 2023 20:25:39 +0100 Subject: Added test for dynamic indexing within struct members --- tests/svtypes/struct_dynamic_range.sv | 67 +++++++++++++++++++++++++++++++++++ tests/svtypes/struct_dynamic_range.ys | 4 +++ 2 files changed, 71 insertions(+) create mode 100644 tests/svtypes/struct_dynamic_range.sv create mode 100644 tests/svtypes/struct_dynamic_range.ys 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 -- cgit v1.2.3