From b8ddeb88480b6087528be6405183496af4d527c2 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Fri, 23 Dec 2022 20:57:31 -0800 Subject: [PATCH 1/5] vvp: Handle null-bytes in the string literal VPI support The VPI API for string literals does not correctly handle the case where a null-byte ('\0') appears in the string literal. It uses strlen() to calculate the length of the literal, which will give the wrong result if there is a null-byte in the string literal. Instead of using strlen() use the stored length to fix this. In addition when formatting a string literal as a string ignore any null-bytes. The LRM is not entirely clear what should happen to null-bytes when formatting a value as a string. But the behavior of ignoring the null-bytes is consistent with the rules of SystemVerilog for converting a string literal to a SV string. This problem can occur when a string literal gets null-byte left-padded due to width of its context of its expression, but then optimization removes part of the expression and only leaves the padded string literal. E.g. ``` $display(0 ? "Yes" : "No"); ``` will be transformed into ``` $display("\000No"); ``` There is also one subtle change in behavior associated with this. The empty string ("") is supposed to be equivalent to 8'h00. So e.g. `$display(":%s:", "")` should print ": :" since the width of the empty string is 1 byte and the %s modifier prints a string with the width of the value, left-padding with spaces if necessary. The current implementation will print "::" though. This change requires to update the marco_with_args gold file. Signed-off-by: Lars-Peter Clausen --- ivtest/gold/macro_with_args.gold | 2 +- vvp/vpi_const.cc | 19 ++++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/ivtest/gold/macro_with_args.gold b/ivtest/gold/macro_with_args.gold index c13571aa5..b0a34d36c 100644 --- a/ivtest/gold/macro_with_args.gold +++ b/ivtest/gold/macro_with_args.gold @@ -1,4 +1,4 @@ first..last first,last last..first -(a)..(c) (a,b,c) (c)..(a) +(a )..(c ) (a,b,c) (c )..(a ) sumsqr(3,4) = 25 sumsqr(5,12) = 169 diff --git a/vvp/vpi_const.cc b/vvp/vpi_const.cc index 3fccb82b8..175c4e5fe 100644 --- a/vvp/vpi_const.cc +++ b/vvp/vpi_const.cc @@ -93,7 +93,7 @@ int __vpiStringConst::vpi_get(int code) { switch (code) { case vpiSize: - return strlen(value_)*8; + return value_len_ * 8; case vpiSigned: return 0; @@ -121,7 +121,7 @@ void __vpiStringConst::vpi_get_value(p_vpi_value vp) { unsigned uint_value; p_vpi_vecval vecp; - unsigned size = strlen(value_); + unsigned size = value_len_; char*rbuf = 0; char*cp; @@ -131,9 +131,22 @@ void __vpiStringConst::vpi_get_value(p_vpi_value vp) vp->format = vpiStringVal; // fallthrough case vpiStringVal: + cp = value_; rbuf = (char *) need_result_buf(size + 1, RBUF_VAL); - strcpy(rbuf, value_); vp->value.str = rbuf; + + for (unsigned int i = 0; i < size; i++) { + // Ignore leading null-bytes and replace other null-bytes with space. + // The LRM is not entirely clear on how null bytes should be handled. + // This is the implementation chosen for iverilog. + if (*cp) + *rbuf++ = *cp; + else if (rbuf != vp->value.str) + *rbuf++ = ' '; + + cp++; + } + *rbuf = '\0'; break; case vpiDecStrVal: From 3cfbd7345f0f7ef895d709bbbad6c996be748338 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Mon, 2 Jan 2023 15:33:51 -0800 Subject: [PATCH 2/5] vvp: Only ignore leading null-bytes when reading as string through VPI Currently when reading a number literal through the VPI API as a vpiStringVal all null-bytes in the literal get ignored. This behavior is different from when reading a signal through the VPI API as a vpiStringVal. The latter will only ignore leading null-bytes and replace other null-bytes with a space. E.g. the following two will print different values. ``` $display("%s", "a\000b"); // -> " ab" reg [23:0] x = "a\000b"; $display("%s", x); // -> "a b" ``` For consistency modify the number literal formatting code so that it has the same behavior as the signal value formatting code and only replaces leading null-bytes. Signed-off-by: Lars-Peter Clausen --- vvp/vpi_priv.cc | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/vvp/vpi_priv.cc b/vvp/vpi_priv.cc index ff11cfe9e..1de770fd4 100644 --- a/vvp/vpi_priv.cc +++ b/vvp/vpi_priv.cc @@ -635,6 +635,8 @@ static void vec4_get_value_string(const vvp_vector4_t&word_val, unsigned width, if (char_val != 0) *cp++ = char_val; + else if (cp != rbuf) + *cp++ = ' '; } for (unsigned idx = 0 ; idx < nchar ; idx += 1) { @@ -645,8 +647,13 @@ static void vec4_get_value_string(const vvp_vector4_t&word_val, unsigned width, if (val == BIT4_1) char_val |= 1 << bdx; } + // Ignore leading null-bytes and replace other null-bytes with space. + // The LRM is not entirely clear on how null bytes should be handled. + // This is the implementation chosen for iverilog. if (char_val != 0) *cp++ = char_val; + else if (cp != rbuf) + *cp++ = ' '; } *cp = 0; From 2e12e47a2ba95a5d4628ba32b6aeef7133bfa333 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Mon, 2 Jan 2023 11:55:26 -0800 Subject: [PATCH 3/5] tgt-vlog95: Don't strip leading null-bytes from string literals When a string literal is used in a context where it needs to be wider than it is it will get left-padded with null-bytes. When the vlog95 backend emits the string literal it will strip the leading null-bytes as it results in much more legible code. Unfortunately there are some corner cases where this results in a change of behavior of the generated code compared to the original. E.g. if the context that caused the width expansion has been removed by optimization. `$display(0 ? "Yes" : "No")` should print " No" due to width expansion, but when running through the vlog95 backend it will print "No". Another scenario where there is a change in behavior is when a null byte was explicitly added at the front of a string literal. E.g. $bits("\000ab") should print 24, but will print 16 when running through the vlog95 backend. To mitigate this remove the stripping of the leading null-bytes from the vlog95 backend. This results in slightly less legible code being generated in some cases, but makes sure that the code is always correct. Signed-off-by: Lars-Peter Clausen --- tgt-vlog95/numbers.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tgt-vlog95/numbers.c b/tgt-vlog95/numbers.c index 4ba0ee7e8..5dee05a00 100644 --- a/tgt-vlog95/numbers.c +++ b/tgt-vlog95/numbers.c @@ -346,13 +346,9 @@ static void remove_two_chars(char* str) void emit_string(const char* string) { char *buffer = strdup(string); - char *bptr = buffer; char *cptr; fprintf(vlog_out, "\""); - /* Prune any leading escaped NULL bytes. */ - while ((bptr[0] == '\\') && (bptr[1] == '0') && - (bptr[2] == '0') && (bptr[3] == '0')) bptr += 4; - for (cptr = bptr; *cptr; cptr += 1) { + for (cptr = buffer; *cptr; cptr += 1) { if (*cptr == '\\') { /* Replace any \011 with \t */ if ((cptr[1] == '0') && (cptr[2] == '1') && @@ -381,7 +377,7 @@ void emit_string(const char* string) } else cptr += 3; } } - if (*bptr) fprintf(vlog_out, "%s", bptr); + if (*buffer) fprintf(vlog_out, "%s", buffer); free(buffer); fprintf(vlog_out, "\""); } From 56efec8ed19c3584829feea00ce57d3ec4203e54 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Mon, 2 Jan 2023 15:59:13 -0800 Subject: [PATCH 4/5] tgt-vlog95: Don't strip null-bytes from string literals in structural elements The vlog95 backend currently strips null-bytes from strings in structural elements. E.g. `assign y = "a\000b"` gets translated to `assign y = "ab"`. This changes the behavior of the generated output compared to the input. Don't ignore the null-bytes to make sure the behavior stays the same. Signed-off-by: Lars-Peter Clausen --- tgt-vlog95/misc.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/tgt-vlog95/misc.c b/tgt-vlog95/misc.c index 8c2b8c2c8..248dfeb3b 100644 --- a/tgt-vlog95/misc.c +++ b/tgt-vlog95/misc.c @@ -512,8 +512,6 @@ static void emit_number_as_string(ivl_net_const_t net_const) val |= (bits[idx-bit] == '1') ? 1 << (7-bit) : 0x00; } - /* Skip any NULL bytes. */ - if (val == 0) continue; /* Print some values that can be escaped. */ if (val == '"') fprintf(vlog_out, "\\\""); else if (val == '\\') fprintf(vlog_out, "\\\\"); From 71bb0115975ccb73efe6d5a93ec6ac9e9c106cec Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Sun, 1 Jan 2023 18:45:10 -0800 Subject: [PATCH 5/5] Add regression tests for string formatting of null-bytes through VPI Check that null-bytes get removed when reading a value through the VPI API as a vpiStringVal. Also check that null-bytes are not removed from string literals when string literals are read through the VPI API as a non vpiStringVal. Signed-off-by: Lars-Peter Clausen --- ivtest/gold/br_gh827.gold | 4 +++ ivtest/ivltests/br_gh827.v | 9 ++++++ ivtest/ivltests/string13.v | 58 ++++++++++++++++++++++++++++++++++++++ ivtest/ivltests/string14.v | 38 +++++++++++++++++++++++++ ivtest/regress-vlg.list | 3 ++ 5 files changed, 112 insertions(+) create mode 100644 ivtest/gold/br_gh827.gold create mode 100644 ivtest/ivltests/br_gh827.v create mode 100644 ivtest/ivltests/string13.v create mode 100644 ivtest/ivltests/string14.v diff --git a/ivtest/gold/br_gh827.gold b/ivtest/gold/br_gh827.gold new file mode 100644 index 000000000..30d8061ee --- /dev/null +++ b/ivtest/gold/br_gh827.gold @@ -0,0 +1,4 @@ +Const Positive yes +Const Negative NO +Var Positive yes +Var Negative NO diff --git a/ivtest/ivltests/br_gh827.v b/ivtest/ivltests/br_gh827.v new file mode 100644 index 000000000..927d666cf --- /dev/null +++ b/ivtest/ivltests/br_gh827.v @@ -0,0 +1,9 @@ +module test; + reg expected = 1; + initial begin + $display("Const Positive %s", 1 ? "yes" : "NO"); + $display("Const Negative %s", !1 ? "yes" : "NO"); + $display("Var Positive %s", expected ? "yes" : "NO"); + $display("Var Negative %s", !expected ? "yes" : "NO"); + end +endmodule diff --git a/ivtest/ivltests/string13.v b/ivtest/ivltests/string13.v new file mode 100644 index 000000000..3ade099ae --- /dev/null +++ b/ivtest/ivltests/string13.v @@ -0,0 +1,58 @@ +// Check that null-bytes are handled consistently between string literals, +// number literals and signals of all kinds, especially when formatting as a +// string. + +module test; + + reg failed = 1'b0; + + `define check(val, exp) \ + if (val != exp) begin \ + $display("FAILED(%0d): Expected '%0s', got '%0s'.", `__LINE__, exp, val); \ + failed = 1'b1; \ + end + + reg [255:0] s; + reg [31:0] x; + reg [31:0] y[1:0]; + wire [31:0] z; + wire [31:0] w; + + assign z = "\000a\000b"; + assign w = 32'h00610062; + + initial begin + $sformat(s, ":%x:%0x:%s:%0s:", "\000a\000b", "\000a\000b", "\000a\000b", "\000a\000b"); + `check(s, ":00610062:610062: a b:a b:") + $sformat(s, ":%x:%0x:%s:%0s:", 32'h00610062, 32'h00610062, 32'h00610062, 32'h00610062); + `check(s, ":00610062:610062: a b:a b:") + + x = "\000a\000b"; + $sformat(s, ":%x:%0x:%s:%0s:", x, x, x, x); + `check(s, ":00610062:610062: a b:a b:") + + x = 32'h00610062; + $sformat(s, ":%x:%0x:%s:%0s:", x, x, x, x); + `check(s, ":00610062:610062: a b:a b:") + + y[0] = "\000a\000b"; + $sformat(s, ":%x:%0x:%s:%0s:", y[0], y[0], y[0], y[0]); + `check(s, ":00610062:610062: a b:a b:") + + y[1] = 32'h00610062; + $sformat(s, ":%x:%0x:%s:%0s:", y[1], y[1], y[1], y[1]); + `check(s, ":00610062:610062: a b:a b:") + + $sformat(s, ":%x:%0x:%s:%0s:", z, z, z, z); + `check(s, ":00610062:610062: a b:a b:") + + $sformat(s, ":%x:%0x:%s:%0s:", w, w, w, w); + `check(s, ":00610062:610062: a b:a b:") + + + if (!failed) begin + $display("PASSED"); + end + end + +endmodule diff --git a/ivtest/ivltests/string14.v b/ivtest/ivltests/string14.v new file mode 100644 index 000000000..882e9b707 --- /dev/null +++ b/ivtest/ivltests/string14.v @@ -0,0 +1,38 @@ +// Check that the empty string "" is equivalent to 8'h00 + +module test; + + reg failed = 1'b0; + + `define check(val, exp) \ + if (val != exp) begin \ + $display("FAILED(%0d): Expected '%0s', got '%0s'.", `__LINE__, exp, val); \ + failed = 1'b1; \ + end + + reg [47:0] s; + reg [7:0] x; + wire [7:0] y; + + assign y = ""; + + initial begin + `check("", 8'h00); + `check($bits(""), 8); + + $sformat(s, ":%s:%0s:", "", ""); + `check(s, ": ::"); + + x = 8'h00; + $sformat(s, ":%s:%0s:", x, x); + `check(s, ": ::"); + + $sformat(s, ":%s:%0s:", y, y); + `check(s, ": ::"); + + if (!failed) begin + $display("PASSED"); + end + end + +endmodule diff --git a/ivtest/regress-vlg.list b/ivtest/regress-vlg.list index 977970c83..b4db8f2bc 100644 --- a/ivtest/regress-vlg.list +++ b/ivtest/regress-vlg.list @@ -355,6 +355,7 @@ br_gh782e normal ivltests br_gh782f normal ivltests br_gh788 normal,-gno-io-range-error,-Wno-anachronisms ivltests gold=br_gh788.gold br_gh793 normal ivltests +br_gh827 normal ivltests gold=br_gh827.gold br_ml20150315 normal ivltests gold=br_ml_20150315.gold br_ml20150321 CE ivltests br_mw20171108 normal ivltests @@ -1646,6 +1647,8 @@ string9 normal ivltests gold=string9.gold string10 normal ivltests gold=string10.gold string11 normal ivltests gold=string11.gold string12 normal ivltests +string13 normal ivltests +string14 normal ivltests supply1 normal ivltests supply2 normal ivltests switch_primitives normal ivltests gold=switch_primitives.gold