From 71235f9b4859ff9c166101bf796eda6040077031 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Povi=C5=A1er?= Date: Thu, 8 Feb 2024 14:57:28 +0100 Subject: [PATCH 1/2] fmt: Catch `$time` argument with mismatched specifier --- kernel/fmt.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/kernel/fmt.cc b/kernel/fmt.cc index 18eb7cb71..92a26191e 100644 --- a/kernel/fmt.cc +++ b/kernel/fmt.cc @@ -431,9 +431,10 @@ void Fmt::parse_verilog(const std::vector &args, bool sformat_lik } break; } - if (i == fmt.size()) { + if (i == fmt.size()) log_file_error(fmtarg->filename, fmtarg->first_line, "System task `%s' called with incomplete format specifier in argument %zu.\n", task_name.c_str(), fmtarg - args.begin() + 1); - } + if (arg->type == VerilogFmtArg::TIME && part.type != FmtPart::VLOG_TIME) + log_file_error(fmtarg->filename, fmtarg->first_line, "System task `%s' called with format character `%c' in argument %zu, but the argument is $time or $realtime.\n", task_name.c_str(), fmt[i], fmtarg - args.begin() + 1); if (part.padding == '\0') part.padding = (has_leading_zero && part.justify == FmtPart::RIGHT) ? '0' : ' '; From 0635df4ee53c1de9e65b1a306340cb23ac052188 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Povi=C5=A1er?= Date: Thu, 8 Feb 2024 15:03:38 +0100 Subject: [PATCH 2/2] fmt: Extend string handling for SystemVerilog This makes for a distinction between a string argument from a quoted literal, and a string argument from a variable or other expression. --- frontends/ast/genrtlil.cc | 2 -- kernel/fmt.cc | 17 ++++++++++++++--- kernel/fmt.h | 15 ++++++++++++++- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/frontends/ast/genrtlil.cc b/frontends/ast/genrtlil.cc index 03697ebf3..47c24bc86 100644 --- a/frontends/ast/genrtlil.cc +++ b/frontends/ast/genrtlil.cc @@ -771,8 +771,6 @@ struct AST_INTERNAL::ProcessGenerator if (node->type == AST_CONSTANT && node->is_string) { arg.type = VerilogFmtArg::STRING; arg.str = node->bitsAsConst().decode_string(); - // and in case this will be used as an argument... - arg.sig = node->bitsAsConst(); arg.signed_ = false; } else if (node->type == AST_IDENTIFIER && node->str == "$time") { arg.type = VerilogFmtArg::TIME; diff --git a/kernel/fmt.cc b/kernel/fmt.cc index 92a26191e..825157c08 100644 --- a/kernel/fmt.cc +++ b/kernel/fmt.cc @@ -337,7 +337,7 @@ void Fmt::parse_verilog(const std::vector &args, bool sformat_lik } case VerilogFmtArg::STRING: { - if (arg == args.begin() || !sformat_like) { + if ((arg == args.begin() || !sformat_like) && !arg->str.empty()) { const auto fmtarg = arg; const std::string &fmt = fmtarg->str; FmtPart part = {}; @@ -366,8 +366,19 @@ void Fmt::parse_verilog(const std::vector &args, bool sformat_lik if (++arg == args.end()) { log_file_error(fmtarg->filename, fmtarg->first_line, "System task `%s' called with fewer arguments than the format specifiers in argument %zu require.\n", task_name.c_str(), fmtarg - args.begin() + 1); } - part.sig = arg->sig; - part.signed_ = arg->signed_; + switch (arg->type) { + case VerilogFmtArg::STRING: + part.sig = arg->str.empty() ? arg->sig : Const(arg->str); + part.signed_ = false; + break; + case VerilogFmtArg::INTEGER: + part.sig = arg->sig; + part.signed_ = arg->signed_; + break; + default: + // requires `%t`/`%T` which is enforced later on + break; + } for (; i < fmt.size(); i++) { if (fmt[i] == '-') { diff --git a/kernel/fmt.h b/kernel/fmt.h index 3bedb786e..1bbceaa01 100644 --- a/kernel/fmt.h +++ b/kernel/fmt.h @@ -26,6 +26,17 @@ YOSYS_NAMESPACE_BEGIN // Verilog format argument, such as the arguments in: // $display("foo %d bar %01x", 4'b0, $signed(2'b11)) +// +// Argument mapping: +// +// (1) quoted string literal -> STRING, literal in `str` +// (2) type `string` or unpacked `byte` array -> STRING, value in `sig` (SystemVerilog extension) +// (3) `$time` `$realtime` -> TIME, `realtime` selects between the two +// (4) any other expression -> INTEGER, value in `sig` +// +// We need to distinguish (1) and (2) because that influences interpretation +// of escape characters (only interpreted in (1)). +// struct VerilogFmtArg { enum { STRING = 0, @@ -40,8 +51,10 @@ struct VerilogFmtArg { // STRING type std::string str; - // INTEGER type + // INTEGER/STRING type RTLIL::SigSpec sig; + + // INTEGER bool signed_ = false; // TIME type