WIP: [SDC plugin] Improve error messages

sdc-plugin/sdc.cc

@@ -33,7 +33,10 @@ USING_YOSYS_NAMESPACE
 PRIVATE_NAMESPACE_BEGIN
 
 struct ReadSdcCmd : public Frontend {
-    ReadSdcCmd() : Frontend("sdc", "Read SDC file") {}
+    ReadSdcCmd(std::string &file_name, size_t &line_number) : Frontend("sdc", "Read SDC file"), file_name(file_name), line_number(line_number) {}
+
+    std::string &file_name;
+    size_t &line_number;
 
     void help() override
     {
@@ -52,11 +55,16 @@ struct ReadSdcCmd : public Frontend {
         log("\nReading clock constraints file(SDC)\n\n");
         size_t argidx = 1;
         extra_args(f, filename, args, argidx);
-        std::string content{std::istreambuf_iterator<char>(*f), std::istreambuf_iterator<char>()};
-        log("%s\n", content.c_str());
+        file_name = filename;
+        line_number = 0;
         Tcl_Interp *interp = yosys_get_tcl_interp();
-        if (Tcl_EvalFile(interp, args[argidx].c_str()) != TCL_OK) {
-            log_cmd_error("TCL interpreter returned an error: %s\n", Tcl_GetStringResult(interp));
+        while (!f->eof()) {
+            std::string line;
+            std::getline(*f, line);
+            line_number++;
+            if (Tcl_Eval(interp, line.c_str()) != TCL_OK) {
+                log_file_error(file_name, line_number, "%s: TCL interpreter returned an error: %s\n", pass_name.c_str(), Tcl_GetStringResult(interp));
+            }
         }
     }
 };
@@ -100,7 +108,13 @@ struct WriteSdcCmd : public Backend {
 };
 
 struct CreateClockCmd : public Pass {
-    CreateClockCmd() : Pass("create_clock", "Create clock object") {}
+    CreateClockCmd(const std::string &file_name, size_t &line_number)
+        : Pass("create_clock", "Create clock object"), file_name(file_name), line_number(line_number)
+    {
+    }
+
+    const std::string &file_name;
+    const size_t &line_number;
 
     void help() override
     {
@@ -127,7 +141,8 @@ struct CreateClockCmd : public Pass {
         float falling_edge(0);
         float period(0);
         if (args.size() < 4) {
-            log_cmd_error("Incorrect number of arguments\n");
+            log_file_error(file_name, line_number, "%s: Found only %ld arguments, but a minimum of 3 are required.\n", pass_name.c_str(),
+                           args.size() - 1);
         }
         for (argidx = 1; argidx < args.size(); argidx++) {
             std::string arg = args[argidx];
@@ -149,8 +164,16 @@ struct CreateClockCmd : public Pass {
             }
             break;
         }
+        if (argidx == args.size()) {
+            log_file_error(file_name, line_number, "%s: No target signal was provided.\n", pass_name.c_str());
+        }
         if (period <= 0) {
-            log_cmd_error("Incorrect period value\n");
+            log_file_error(file_name, line_number, "%s: Found non-positive period value of %f, periods must be positive and greater than zero.\n",
+                           pass_name.c_str(), period);
+        }
+        if (!is_waveform_specified) {
+            rising_edge = 0;
+            falling_edge = period / 2;
         }
         // Add "w:" prefix to selection arguments to enforce wire object
         // selection
@@ -177,10 +200,6 @@ struct CreateClockCmd : public Pass {
         if (name.empty()) {
             name = RTLIL::unescape_id(selected_wires.at(0)->name);
         }
-        if (!is_waveform_specified) {
-            rising_edge = 0;
-            falling_edge = period / 2;
-        }
         Clock::Add(name, selected_wires, period, rising_edge, falling_edge, Clock::EXPLICIT);
     }
 
@@ -331,7 +350,9 @@ struct PropagateClocksCmd : public Pass {
 class SdcPlugin
 {
   public:
-    SdcPlugin() : write_sdc_cmd_(sdc_writer_), set_false_path_cmd_(sdc_writer_), set_max_delay_cmd_(sdc_writer_), set_clock_groups_cmd_(sdc_writer_)
+    SdcPlugin()
+        : read_sdc_cmd_(file_name, line_number), write_sdc_cmd_(sdc_writer_), create_clock_cmd_(file_name, line_number),
+          set_false_path_cmd_(sdc_writer_), set_max_delay_cmd_(sdc_writer_), set_clock_groups_cmd_(sdc_writer_)
     {
         log("Loaded SDC plugin\n");
     }
@@ -345,6 +366,9 @@ class SdcPlugin
     SetMaxDelay set_max_delay_cmd_;
     SetClockGroups set_clock_groups_cmd_;
 
+    std::string file_name;
+    size_t line_number;
+
   private:
     SdcWriter sdc_writer_;
 } SdcPlugin;

sdc-plugin/tests/Makefile

@@ -32,7 +32,9 @@ TESTS = abc9 \
 	period_check \
 	waveform_check \
 	period_format_check \
-	get_clocks
+	get_clocks \
+	sdc_errors_missing_arguments
+
 
 UNIT_TESTS = escaping
 
@@ -57,4 +59,6 @@ waveform_check_verify = true
 waveform_check_negative = 1
 period_format_check_verify = true
 period_format_check_negative = 1
+sdc_errors_missing_arguments_verify = tail -n 1 sdc_errors_missing_arguments/sdc_errors_missing_arguments.log | grep sdc_errors_missing_arguments.input.sdc:1 > /dev/null
+sdc_errors_missing_arguments_negative = 1
 get_clocks_verify = $(call diff_test,get_clocks,txt)

sdc-plugin/tests/sdc_errors_missing_arguments/sdc_errors_missing_arguments.input.sdc

@@ -0,0 +1 @@
+create_clock -period 10.0

sdc-plugin/tests/sdc_errors_missing_arguments/sdc_errors_missing_arguments.tcl

@@ -0,0 +1,13 @@
+yosys -import
+if { [info procs read_sdc] == {} } { plugin -i sdc }
+yosys -import  ;# ingest plugin commands
+
+read_verilog $::env(DESIGN_TOP).v
+read_verilog -specify -lib -D_EXPLICIT_CARRY +/xilinx/cells_sim.v
+read_verilog -lib +/xilinx/cells_xtra.v
+hierarchy -check -auto-top
+# Start flow after library reading
+synth_xilinx -flatten -abc9 -nosrl -nodsp -iopad -run prepare:check
+
+# Read the design's timing constraints
+read_sdc $::env(DESIGN_TOP).input.sdc

sdc-plugin/tests/sdc_errors_missing_arguments/sdc_errors_missing_arguments.v

@@ -0,0 +1,66 @@
+// Copyright (C) 2020-2021  The SymbiFlow Authors.
+//
+// Use of this source code is governed by a ISC-style
+// license that can be found in the LICENSE file or at
+// https://opensource.org/licenses/ISC
+//
+// SPDX-License-Identifier:ISC
+
+module top (
+    input clk,
+    input clk2,
+    input [1:0] in,
+    output [5:0] out
+);
+
+  reg [1:0] cnt = 0;
+  wire clk_int_1, clk_int_2;
+  IBUF ibuf_proxy (
+      .I(clk),
+      .O(ibuf_proxy_out)
+  );
+  IBUF ibuf_inst (
+      .I(ibuf_proxy_out),
+      .O(ibuf_out)
+  );
+  assign clk_int_1 = ibuf_out;
+  assign clk_int_2 = clk_int_1;
+
+  always @(posedge clk_int_2) begin
+    cnt <= cnt + 1;
+  end
+
+  middle middle_inst_1 (
+      .clk(ibuf_out),
+      .out(out[2])
+  );
+  middle middle_inst_2 (
+      .clk(clk_int_1),
+      .out(out[3])
+  );
+  middle middle_inst_3 (
+      .clk(clk_int_2),
+      .out(out[4])
+  );
+  middle middle_inst_4 (
+      .clk(clk2),
+      .out(out[5])
+  );
+
+  assign out[1:0] = {cnt[0], in[0]};
+endmodule
+
+module middle (
+    input  clk,
+    output out
+);
+
+  reg [1:0] cnt = 0;
+  wire clk_int;
+  assign clk_int = clk;
+  always @(posedge clk_int) begin
+    cnt <= cnt + 1;
+  end
+
+  assign out = cnt[0];
+endmodule