From c85589371a2b469ad587733f4a6ca90a1b90f239 Mon Sep 17 00:00:00 2001 From: Yutetsu TAKATSUKASA Date: Sat, 1 Aug 2020 20:16:15 +0900 Subject: [PATCH] Fix protect-lib without sequential logic (#2492) * add a test for protect-lib without sequential logic that cause the follwoing error. %Error: obj_vlt/t_prot_lib_comb/secret/secret.sv:78:14: syntax error, unexpected ')' 78 | always @() begin | ^ * protect-lib wrapper must contain sequential signal related statements only if the protected design is a sequential logic. Diff looks big, but actually just add "if (m_hasClk)" condition. Signed-off-by: Yutetsu TAKATSUKASA --- src/V3ProtectLib.cpp | 161 +++++++++++++++++++----------- test_regress/t/t_prot_lib_comb.pl | 69 +++++++++++++ test_regress/t/t_prot_lib_comb.v | 56 +++++++++++ 3 files changed, 225 insertions(+), 61 deletions(-) create mode 100755 test_regress/t/t_prot_lib_comb.pl create mode 100644 test_regress/t/t_prot_lib_comb.v diff --git a/src/V3ProtectLib.cpp b/src/V3ProtectLib.cpp index 06fd9d726..740381e71 100644 --- a/src/V3ProtectLib.cpp +++ b/src/V3ProtectLib.cpp @@ -60,6 +60,7 @@ private: string m_libName; string m_topName; bool m_foundTop; // Have seen the top module + bool m_hasClk; // True if the top module has sequential logic // VISITORS virtual void visit(AstNetlist* nodep) VL_OVERRIDE { @@ -79,6 +80,8 @@ private: UASSERT_OBJ(!m_foundTop, nodep, "Multiple root modules"); } FileLine* fl = nodep->fileline(); + // Need to know the existence of clk before createSvFile() + m_hasClk = checkIfClockExists(nodep); createSvFile(fl); createCppFile(fl); @@ -162,14 +165,16 @@ private: txtp->addNodep(m_comboPortsp); txtp->addText(fl, ");\n\n"); seqComment(txtp, fl); - m_seqPortsp = new AstTextBlock(fl, - "import \"DPI-C\" function longint " + m_libName - + "_protectlib_seq_update" - "(\n", - false, true); - m_seqPortsp->addText(fl, "chandle handle__V\n"); - txtp->addNodep(m_seqPortsp); - txtp->addText(fl, ");\n\n"); + if (m_hasClk) { + m_seqPortsp = new AstTextBlock(fl, + "import \"DPI-C\" function longint " + m_libName + + "_protectlib_seq_update" + "(\n", + false, true); + m_seqPortsp->addText(fl, "chandle handle__V\n"); + txtp->addNodep(m_seqPortsp); + txtp->addText(fl, ");\n\n"); + } comboIgnoreComment(txtp, fl); m_comboIgnorePortsp = new AstTextBlock(fl, "import \"DPI-C\" function void " + m_libName @@ -192,7 +197,7 @@ private: m_tmpDeclsp = new AstTextBlock(fl); txtp->addNodep(m_tmpDeclsp); txtp->addText(fl, "\ntime last_combo_seqnum__V;\n"); - txtp->addText(fl, "time last_seq_seqnum__V;\n\n"); + if (m_hasClk) { txtp->addText(fl, "time last_seq_seqnum__V;\n\n"); } // CPP hash value addComment(txtp, fl, "Hash value to make sure this file and the corresponding"); @@ -222,33 +227,41 @@ private: txtp->addText(fl, "end\n\n"); // Sequential process - addComment(txtp, fl, "Evaluate clock edges"); - m_clkSensp = new AstTextBlock(fl, "always @(", false, true); - txtp->addNodep(m_clkSensp); - txtp->addText(fl, ") begin\n"); - m_comboIgnoreParamsp - = new AstTextBlock(fl, m_libName + "_protectlib_combo_ignore(\n", false, true); - m_comboIgnoreParamsp->addText(fl, "handle__V\n"); - txtp->addNodep(m_comboIgnoreParamsp); - txtp->addText(fl, ");\n"); - m_seqParamsp = new AstTextBlock( - fl, "last_seq_seqnum__V <= " + m_libName + "_protectlib_seq_update(\n", false, true); - m_seqParamsp->addText(fl, "handle__V\n"); - txtp->addNodep(m_seqParamsp); - txtp->addText(fl, ");\n"); - m_nbAssignsp = new AstTextBlock(fl); - txtp->addNodep(m_nbAssignsp); - txtp->addText(fl, "end\n\n"); + if (m_hasClk) { + addComment(txtp, fl, "Evaluate clock edges"); + m_clkSensp = new AstTextBlock(fl, "always @(", false, true); + txtp->addNodep(m_clkSensp); + txtp->addText(fl, ") begin\n"); + m_comboIgnoreParamsp + = new AstTextBlock(fl, m_libName + "_protectlib_combo_ignore(\n", false, true); + m_comboIgnoreParamsp->addText(fl, "handle__V\n"); + txtp->addNodep(m_comboIgnoreParamsp); + txtp->addText(fl, ");\n"); + m_seqParamsp = new AstTextBlock( + fl, "last_seq_seqnum__V <= " + m_libName + "_protectlib_seq_update(\n", false, + true); + m_seqParamsp->addText(fl, "handle__V\n"); + txtp->addNodep(m_seqParamsp); + txtp->addText(fl, ");\n"); + m_nbAssignsp = new AstTextBlock(fl); + txtp->addNodep(m_nbAssignsp); + txtp->addText(fl, "end\n\n"); + } // Select between combinatorial and sequential results addComment(txtp, fl, "Select between combinatorial and sequential results"); txtp->addText(fl, "always @(*) begin\n"); - m_seqAssignsp = new AstTextBlock(fl, "if (last_seq_seqnum__V > " - "last_combo_seqnum__V) begin\n"); - txtp->addNodep(m_seqAssignsp); - m_comboAssignsp = new AstTextBlock(fl, "end else begin\n"); - txtp->addNodep(m_comboAssignsp); - txtp->addText(fl, "end\n"); + if (m_hasClk) { + m_seqAssignsp = new AstTextBlock(fl, "if (last_seq_seqnum__V > " + "last_combo_seqnum__V) begin\n"); + txtp->addNodep(m_seqAssignsp); + m_comboAssignsp = new AstTextBlock(fl, "end else begin\n"); + txtp->addNodep(m_comboAssignsp); + txtp->addText(fl, "end\n"); + } else { + m_comboAssignsp = new AstTextBlock(fl, ""); + txtp->addNodep(m_comboAssignsp); + } txtp->addText(fl, "end\n\n"); // Final @@ -327,19 +340,21 @@ private: txtp->addText(fl, "return handlep__V->m_seqnum++;\n"); txtp->addText(fl, "}\n\n"); - seqComment(txtp, fl); - m_cSeqParamsp = new AstTextBlock( - fl, "long long " + m_libName + "_protectlib_seq_update(\n", false, true); - m_cSeqParamsp->addText(fl, "void* vhandlep__V\n"); - txtp->addNodep(m_cSeqParamsp); - txtp->addText(fl, ")\n"); - m_cSeqClksp = new AstTextBlock(fl, "{\n"); - castPtr(fl, m_cSeqClksp); - txtp->addNodep(m_cSeqClksp); - m_cSeqOutsp = new AstTextBlock(fl, "handlep__V->eval();\n"); - txtp->addNodep(m_cSeqOutsp); - txtp->addText(fl, "return handlep__V->m_seqnum++;\n"); - txtp->addText(fl, "}\n\n"); + if (m_hasClk) { + seqComment(txtp, fl); + m_cSeqParamsp = new AstTextBlock( + fl, "long long " + m_libName + "_protectlib_seq_update(\n", false, true); + m_cSeqParamsp->addText(fl, "void* vhandlep__V\n"); + txtp->addNodep(m_cSeqParamsp); + txtp->addText(fl, ")\n"); + m_cSeqClksp = new AstTextBlock(fl, "{\n"); + castPtr(fl, m_cSeqClksp); + txtp->addNodep(m_cSeqClksp); + m_cSeqOutsp = new AstTextBlock(fl, "handlep__V->eval();\n"); + txtp->addNodep(m_cSeqOutsp); + txtp->addText(fl, "return handlep__V->m_seqnum++;\n"); + txtp->addText(fl, "}\n\n"); + } comboIgnoreComment(txtp, fl); m_cIgnoreParamsp = new AstTextBlock( @@ -369,6 +384,7 @@ private: } if (nodep->direction() == VDirection::INPUT) { if (nodep->isUsedClock() || nodep->attrClocker() == VVarAttrClocker::CLOCKER_YES) { + UASSERT_OBJ(m_hasClk, nodep, "checkIfClockExists() didn't find this clock"); handleClock(nodep); } else { handleDataInput(nodep); @@ -396,8 +412,10 @@ private: FileLine* fl = varp->fileline(); handleInput(varp); m_seqPortsp->addNodep(varp->cloneTree(false)); - m_seqParamsp->addText(fl, varp->name() + "\n"); - m_clkSensp->addText(fl, "edge(" + varp->name() + ")"); + if (m_hasClk) { + m_seqParamsp->addText(fl, varp->name() + "\n"); + m_clkSensp->addText(fl, "edge(" + varp->name() + ")"); + } m_cSeqParamsp->addText(fl, varp->dpiArgType(true, false) + "\n"); m_cSeqClksp->addText(fl, cInputConnection(varp)); } @@ -408,7 +426,7 @@ private: m_comboPortsp->addNodep(varp->cloneTree(false)); m_comboParamsp->addText(fl, varp->name() + "\n"); m_comboIgnorePortsp->addNodep(varp->cloneTree(false)); - m_comboIgnoreParamsp->addText(fl, varp->name() + "\n"); + if (m_hasClk) { m_comboIgnoreParamsp->addText(fl, varp->name() + "\n"); } m_cComboParamsp->addText(fl, varp->dpiArgType(true, false) + "\n"); m_cComboInsp->addText(fl, cInputConnection(varp)); m_cIgnoreParamsp->addText(fl, varp->dpiArgType(true, false) + "\n"); @@ -421,29 +439,49 @@ private: m_modPortsp->addNodep(varp->cloneTree(false)); m_comboPortsp->addNodep(varp->cloneTree(false)); m_comboParamsp->addText(fl, varp->name() + "_combo__V\n"); - m_seqPortsp->addNodep(varp->cloneTree(false)); - m_seqParamsp->addText(fl, varp->name() + "_tmp__V\n"); + if (m_hasClk) { + m_seqPortsp->addNodep(varp->cloneTree(false)); + m_seqParamsp->addText(fl, varp->name() + "_tmp__V\n"); + } AstNodeDType* comboDtypep = varp->dtypep()->cloneTree(false); m_comboDeclsp->addNodep(comboDtypep); m_comboDeclsp->addText(fl, " " + varp->name() + "_combo__V;\n"); - AstNodeDType* seqDtypep = varp->dtypep()->cloneTree(false); - m_seqDeclsp->addNodep(seqDtypep); - m_seqDeclsp->addText(fl, " " + varp->name() + "_seq__V;\n"); + if (m_hasClk) { + AstNodeDType* seqDtypep = varp->dtypep()->cloneTree(false); + m_seqDeclsp->addNodep(seqDtypep); + m_seqDeclsp->addText(fl, " " + varp->name() + "_seq__V;\n"); - AstNodeDType* tmpDtypep = varp->dtypep()->cloneTree(false); - m_tmpDeclsp->addNodep(tmpDtypep); - m_tmpDeclsp->addText(fl, " " + varp->name() + "_tmp__V;\n"); + AstNodeDType* tmpDtypep = varp->dtypep()->cloneTree(false); + m_tmpDeclsp->addNodep(tmpDtypep); + m_tmpDeclsp->addText(fl, " " + varp->name() + "_tmp__V;\n"); - m_nbAssignsp->addText(fl, varp->name() + "_seq__V <= " + varp->name() + "_tmp__V;\n"); - m_seqAssignsp->addText(fl, varp->name() + " = " + varp->name() + "_seq__V;\n"); + m_nbAssignsp->addText(fl, varp->name() + "_seq__V <= " + varp->name() + "_tmp__V;\n"); + m_seqAssignsp->addText(fl, varp->name() + " = " + varp->name() + "_seq__V;\n"); + } m_comboAssignsp->addText(fl, varp->name() + " = " + varp->name() + "_combo__V;\n"); m_cComboParamsp->addText(fl, varp->dpiArgType(true, false) + "\n"); m_cComboOutsp->addText(fl, V3Task::assignInternalToDpi(varp, true, "", "", "handlep__V->")); - m_cSeqParamsp->addText(fl, varp->dpiArgType(true, false) + "\n"); - m_cSeqOutsp->addText(fl, V3Task::assignInternalToDpi(varp, true, "", "", "handlep__V->")); + if (m_hasClk) { + m_cSeqParamsp->addText(fl, varp->dpiArgType(true, false) + "\n"); + m_cSeqOutsp->addText(fl, + V3Task::assignInternalToDpi(varp, true, "", "", "handlep__V->")); + } + } + + static bool checkIfClockExists(AstNodeModule* modp) { + for (AstNode* stmtp = modp->stmtsp(); stmtp; stmtp = stmtp->nextp()) { + if (AstVar* varp = VN_CAST(stmtp, Var)) { + if (varp->direction() == VDirection::INPUT + && (varp->isUsedClock() + || varp->attrClocker() == VVarAttrClocker::CLOCKER_YES)) { + return true; + } + } + } + return false; } public: @@ -475,7 +513,8 @@ public: , m_cIgnoreParamsp(NULL) , m_libName(v3Global.opt.protectLib()) , m_topName(v3Global.opt.prefix()) - , m_foundTop(false) { + , m_foundTop(false) + , m_hasClk(false) { iterate(nodep); } }; diff --git a/test_regress/t/t_prot_lib_comb.pl b/test_regress/t/t_prot_lib_comb.pl new file mode 100755 index 000000000..296873c8f --- /dev/null +++ b/test_regress/t/t_prot_lib_comb.pl @@ -0,0 +1,69 @@ +#!/usr/bin/env perl +# Makes the test run with tracing enabled by default, can be overridden +# with --notrace +unshift(@ARGV, "--trace"); +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2019 by Todd Strader. This program is free software; you +# can redistribute it and/or modify it under the terms of either the GNU +# Lesser General Public License Version 3 or the Perl Artistic License +# Version 2.0. +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios( + vlt => 1, + xsim => 1, + ); + +$Self->{sim_time} = $Self->{benchmark} * 100 if $Self->{benchmark}; + +my $secret_prefix = "secret"; +my $secret_dir = "$Self->{obj_dir}/$secret_prefix"; +mkdir $secret_dir; + +while (1) { + # Always compile the secret file with Verilator no matter what simulator + # we are testing with + run(logfile => "$secret_dir/vlt_compile.log", + cmd => ["perl", + "$ENV{VERILATOR_ROOT}/bin/verilator", + "--prefix", + "Vt_prot_lib_secret", + "-cc", + "-Mdir", + $secret_dir, + "--protect-lib", + $secret_prefix, + "t/t_prot_lib_comb.v"], + verilator_run => 1, + ); + last if $Self->{errors}; + + run(logfile => "$secret_dir/secret_gcc.log", + cmd=>[$ENV{MAKE}, + "-C", + $secret_dir, + "-f", + "Vt_prot_lib_secret.mk"]); + last if $Self->{errors}; + + compile( + verilator_flags2 => ["$secret_dir/secret.sv", + "+define+PROCESS_TOP", + "-LDFLAGS", + "$secret_prefix/libsecret.a"], + xsim_flags2 => ["$secret_dir/secret.sv"], + ); + + execute( + check_finished => 1, + xsim_run_flags2 => ["--sv_lib", + "$secret_dir/libsecret", + "--dpi_absolute"], + ); + + ok(1); + last; +} +1; diff --git a/test_regress/t/t_prot_lib_comb.v b/test_regress/t/t_prot_lib_comb.v new file mode 100644 index 000000000..9b8e9e96d --- /dev/null +++ b/test_regress/t/t_prot_lib_comb.v @@ -0,0 +1,56 @@ +// DESCRIPTION: Verilator: Verilog Test module +// This file ONLY is placed into the Public Domain, for any use, +// without warranty, 2020 by Yutetsu TAKATSUKASA. +// SPDX-License-Identifier: CC0-1.0 + +`ifdef PROCESS_TOP +`define CHECK if (out0 != (in0 ^ in1) || out1 != (in0 | in1) || out2 != (in0 & in1)) begin \ + $display("Mismatch in0:%b in1:%b out0:%b out1:%b out2:%b", in0, in1, out0, out1, out2); \ + $stop; \ + end + +module t (/*AUTOARG*/ + // Inputs + clk + ); + input clk; + + + logic in0, in1; + logic out0, out1, out2; + logic [31:0] count = 0; + // actually XOR and OR and AND + secret i_secret(.in0(in0), .in1(in1), .out0(out0), .out1(out1), .out2(out2)); + + always @(posedge clk) begin + count <= count + 32'd1; + if (count == 32'd1) begin + in0 <= 1'b0; + in1 <= 1'b0; + end else if (count == 32'd2) begin + `CHECK + in0 <= 1'b1; + in1 <= 1'b0; + end else if (count == 32'd3) begin + `CHECK + in0 <= 1'b0; + in1 <= 1'b1; + end else if (count == 32'd4) begin + `CHECK + in0 <= 1'b1; + in1 <= 1'b1; + end else if (count == 32'd5) begin + `CHECK + $write("*-* All Finished *-*\n"); + $finish; + end + end +endmodule + +`else +module secret(input in0, input in1, output out0, output out1, output out2); + assign out0 = in0 ^ in1; + assign out1 = in0 | in1; + assign out2 = in0 & in1; +endmodule +`endif