From 08c6d5bde526a41ad0a70a7debdb7753d9abf243 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Thu, 12 Mar 2026 07:49:21 -0400 Subject: [PATCH] Improve some display error handling (#7212 prep) --- src/V3AstInlines.h | 2 +- src/V3AstNodeExpr.h | 16 +++++++------- src/V3AstNodeStmt.h | 4 ++-- src/V3AstNodes.cpp | 8 +++++++ src/V3EmitCFunc.cpp | 4 ---- src/V3LinkResolve.cpp | 6 +++--- src/V3Number.cpp | 29 +++++++++++++------------- src/V3Width.cpp | 14 +++++++++++-- src/verilog.y | 2 +- test_regress/t/t_display_cwide_bad.out | 5 +++-- test_regress/t/t_display_type_bad.out | 8 +++---- 11 files changed, 57 insertions(+), 41 deletions(-) diff --git a/src/V3AstInlines.h b/src/V3AstInlines.h index 2cc946748..86e6ee9cc 100644 --- a/src/V3AstInlines.h +++ b/src/V3AstInlines.h @@ -153,7 +153,7 @@ AstAlways::AstAlways(AstAssignW* assignp) AstElabDisplay::AstElabDisplay(FileLine* fl, VDisplayType dispType, AstNodeExpr* exprsp) : ASTGEN_SUPER_ElabDisplay(fl) { - addFmtp(new AstSFormatF{fl, AstSFormatF::NoFormat{}, exprsp}); + addFmtp(new AstSFormatF{fl, "", true, exprsp}); m_displayType = dispType; } diff --git a/src/V3AstNodeExpr.h b/src/V3AstNodeExpr.h index 664d7946b..6ca80cc14 100644 --- a/src/V3AstNodeExpr.h +++ b/src/V3AstNodeExpr.h @@ -2157,32 +2157,34 @@ class AstSFormatF final : public AstNodeExpr { // @astgen op2 := scopeNamep : Optional[AstScopeName] string m_text; const bool m_hidden; // Under display, etc - bool m_hasFormat; // Has format code + bool m_exprFormat; // Runtime Node* format, false = text() format code, false = possibly r const char m_missingArgChar; // Format code when argument without format, 'h'/'o'/'b' VTimescale m_timeunit; // Parent module time unit public: - class NoFormat {}; AstSFormatF(FileLine* fl, const string& text, bool hidden, AstNodeExpr* exprsp, char missingArgChar = 'd') : ASTGEN_SUPER_SFormatF(fl) , m_text{text} , m_hidden{hidden} - , m_hasFormat{true} + , m_exprFormat{false} , m_missingArgChar{missingArgChar} { dtypeSetString(); addExprsp(exprsp); } - AstSFormatF(FileLine* fl, NoFormat, AstNodeExpr* exprsp, char missingArgChar = 'd', + class ExprFormat {}; + AstSFormatF(FileLine* fl, ExprFormat, AstNodeExpr* exprsp, char missingArgChar = 'd', bool hidden = true) : ASTGEN_SUPER_SFormatF(fl) , m_text{""} , m_hidden{hidden} - , m_hasFormat{false} + , m_exprFormat{true} , m_missingArgChar{missingArgChar} { dtypeSetString(); addExprsp(exprsp); } ASTGEN_MEMBERS_AstSFormatF; + void dump(std::ostream& str = std::cout) const override; + void dumpJson(std::ostream& str = std::cout) const override; string name() const override VL_MT_STABLE { return m_text; } void name(const string& name) override { m_text = name; } int instrCount() const override { return INSTR_COUNT_PLI; } @@ -2196,8 +2198,8 @@ public: return (name().find("%m") != string::npos || name().find("%M") != string::npos); } bool hidden() const { return m_hidden; } - bool hasFormat() const { return m_hasFormat; } - void hasFormat(bool flag) { m_hasFormat = flag; } + bool exprFormat() const { return m_exprFormat; } + void exprFormat(bool flag) { m_exprFormat = flag; } char missingArgChar() const { return m_missingArgChar; } VTimescale timeunit() const { return m_timeunit; } void timeunit(const VTimescale& flag) { m_timeunit = flag; } diff --git a/src/V3AstNodeStmt.h b/src/V3AstNodeStmt.h index a5662f632..f1198b94b 100644 --- a/src/V3AstNodeStmt.h +++ b/src/V3AstNodeStmt.h @@ -595,7 +595,7 @@ public: char missingArgChar = 'd') : ASTGEN_SUPER_Display(fl) , m_displayType{dispType} { - fmtp(new AstSFormatF{fl, AstSFormatF::NoFormat{}, exprsp, missingArgChar}); + fmtp(new AstSFormatF{fl, AstSFormatF::ExprFormat{}, exprsp, missingArgChar}); this->filep(filep); } ASTGEN_MEMBERS_AstDisplay; @@ -1112,7 +1112,7 @@ public: } AstSFormat(FileLine* fl, AstNodeExpr* lhsp, AstNodeExpr* exprsp, char missingArgChar = 'd') : ASTGEN_SUPER_SFormat(fl) { - fmtp(new AstSFormatF{fl, AstSFormatF::NoFormat{}, exprsp, missingArgChar}); + fmtp(new AstSFormatF{fl, AstSFormatF::ExprFormat{}, exprsp, missingArgChar}); this->lhsp(lhsp); } ASTGEN_MEMBERS_AstSFormat; diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index b13ad7a2a..23706a51e 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -2691,6 +2691,14 @@ void AstPatMember::dumpJson(std::ostream& str) const { } void AstNodeTriop::dump(std::ostream& str) const { this->AstNodeExpr::dump(str); } void AstNodeTriop::dumpJson(std::ostream& str) const { dumpJsonGen(str); } +void AstSFormatF::dump(std::ostream& str) const { + this->AstNodeExpr::dump(str); + if (exprFormat()) str << " [EXPRFMT]"; +} +void AstSFormatF::dumpJson(std::ostream& str) const { + dumpJsonGen(str); + dumpJsonBoolFuncIf(str, exprFormat); +} void AstSel::dump(std::ostream& str) const { this->AstNodeBiop::dump(str); str << " widthConst=" << this->widthConst(); diff --git a/src/V3EmitCFunc.cpp b/src/V3EmitCFunc.cpp index efcf167c4..cd94a1534 100644 --- a/src/V3EmitCFunc.cpp +++ b/src/V3EmitCFunc.cpp @@ -274,10 +274,6 @@ void EmitCFunc::displayArg(AstNode* dispp, AstNode** elistp, bool isScan, const + cvtToStr(VL_VALUE_STRING_MAX_WIDTH) + " bits for any $display-like arguments"); } - if (argp->widthMin() > 8 && fmtLetter == 'c') { - // Technically legal, but surely not what the user intended. - argp->v3warn(WIDTHTRUNC, dispp->verilogKwd() << "of %c format of > 8 bit value"); - } } // string pfmt = "%"+displayFormat(argp, vfmt, fmtLetter)+fmtLetter; string pfmt; diff --git a/src/V3LinkResolve.cpp b/src/V3LinkResolve.cpp index 28cff46c6..ed88f792a 100644 --- a/src/V3LinkResolve.cpp +++ b/src/V3LinkResolve.cpp @@ -436,16 +436,16 @@ class LinkResolveVisitor final : public VNVisitor { if (nodep->user2SetOnce()) return; iterateChildren(nodep); // Cleanup old-school displays without format arguments - if (!nodep->hasFormat()) { + if (nodep->exprFormat()) { UASSERT_OBJ(nodep->text() == "", nodep, - "Non-format $sformatf should have \"\" format"); + "Non-format $sformatf should have text format == \"\""); if (VN_IS(nodep->exprsp(), Const) && VN_AS(nodep->exprsp(), Const)->num().isFromString()) { AstConst* const fmtp = VN_AS(nodep->exprsp()->unlinkFrBack(), Const); nodep->text(fmtp->num().toString()); VL_DO_DANGLING(pushDeletep(fmtp), fmtp); } - nodep->hasFormat(true); + nodep->exprFormat(false); } const string newFormat = expectFormat(nodep, nodep->text(), nodep->exprsp(), false); nodep->text(newFormat); diff --git a/src/V3Number.cpp b/src/V3Number.cpp index 8447ecde9..d98d055f9 100644 --- a/src/V3Number.cpp +++ b/src/V3Number.cpp @@ -613,23 +613,23 @@ string V3Number::ascii(bool prefixed, bool cleanVerilog) const VL_MT_STABLE { bool V3Number::displayedFmtLegal(char format, bool isScan) { // Is this a valid format letter? switch (std::tolower(format)) { - case 'b': return true; - case 'c': return true; - case 'd': return true; // Unsigned decimal - case 'e': return true; - case 'f': return true; - case 'g': return true; - case 'h': return true; - case 'o': return true; + case 'b': return true; // Binary + case 'c': return true; // Character + case 'd': return true; // Decimal; internal: Unsigned decimal + case 'e': return true; // Floating + case 'f': return true; // Floating + case 'g': return true; // Floating + case 'h': return true; // Hex + case 'o': return true; // Octal case 'p': return true; // Pattern - case 's': return true; - case 't': return true; + case 's': return true; // String; internal: number-stored string + case 't': return true; // Time case 'u': return true; // Packed 2-state case 'v': return true; // Strength - case 'x': return true; + case 'x': return true; // Hex case 'z': return true; // Packed 4-state - case '@': return true; // Packed string - case '~': return true; // Signed decimal + case '@': return true; // Internal: Packed string + case '~': return true; // Internal: Signed decimal case '*': return isScan; // $scan ignore argument default: return false; } @@ -748,8 +748,7 @@ string V3Number::displayed(FileLine* fl, const string& vformat) const VL_MT_STAB return str; } // case b/d/x/o case 'c': { - if (width() > 8) - fl->v3warn(WIDTHTRUNC, "$display-like format of %c format of > 8 bit value"); + // V3Width has warning if > 8 bits const unsigned int v = bitsValue(0, 8); str.push_back(static_cast(v)); return str; diff --git a/src/V3Width.cpp b/src/V3Width.cpp index f5c89c503..dfd481ceb 100644 --- a/src/V3Width.cpp +++ b/src/V3Width.cpp @@ -6036,8 +6036,9 @@ class WidthVisitor final : public VNVisitor { void formatNoStringArg(AstNode* argp, char ch) { if (argp && argp->isString()) { - argp->v3error("$display-line format of '%"s + ch + "' illegal with string argument\n" - << argp->warnMore() << "... Suggest use '%s'"); + argp->v3error( + "$display-like format of '%"s + ch + "' illegal with non-numeric argument\n" + << argp->warnMore() << "... Suggest use '%s'"); } } @@ -6092,6 +6093,15 @@ class WidthVisitor final : public VNVisitor { case '%': break; // %% - just output a % case 'm': break; // %m - auto insert "name" case 'l': break; // %m - auto insert "library" + case 'c': { + if (argp->widthMin() > 8) { + // Technically legal, but surely not what the user intended. + argp->v3warn(WIDTHTRUNC, + "$display-like format of %c format of > 8 bit value"); + } + if (argp) argp = VN_AS(argp->nextp(), NodeExpr); + break; + } case 'd': { // Convert decimal to either 'd' or '#' if (argp) { AstNodeExpr* const nextp = VN_AS(argp->nextp(), NodeExpr); diff --git a/src/verilog.y b/src/verilog.y index adf3e6df0..a0f37468e 100644 --- a/src/verilog.y +++ b/src/verilog.y @@ -4498,7 +4498,7 @@ system_f_or_t_expr_call: // IEEE: part of system_tf_call (can be tas | yD_ROSE_GCLK '(' expr ')' { $$ = new AstRose{$1, $3, GRAMMARP->createGlobalClockSenTree($1)}; } | yD_RTOI '(' expr ')' { $$ = new AstRToIS{$1, $3}; } | yD_SAMPLED '(' expr ')' { $$ = new AstSampled{$1, $3}; } - | yD_SFORMATF '(' exprDispList ')' { $$ = new AstSFormatF{$1, AstSFormatF::NoFormat{}, $3, 'd', false}; } + | yD_SFORMATF '(' exprDispList ')' { $$ = new AstSFormatF{$1, AstSFormatF::ExprFormat{}, $3, 'd', false}; } | yD_SHORTREALTOBITS '(' expr ')' { $$ = new AstRealToBits{$1, $3}; UNSUPREAL($1); } | yD_SIGNED '(' expr ')' { $$ = new AstSigned{$1, $3}; } | yD_SIN '(' expr ')' { $$ = new AstSinD{$1, $3}; } diff --git a/test_regress/t/t_display_cwide_bad.out b/test_regress/t/t_display_cwide_bad.out index 900f43601..18a1bfb05 100644 --- a/test_regress/t/t_display_cwide_bad.out +++ b/test_regress/t/t_display_cwide_bad.out @@ -1,6 +1,7 @@ -%Warning-WIDTHTRUNC: t/t_display_cwide_bad.v:10:5: $display-like format of %c format of > 8 bit value +%Warning-WIDTHTRUNC: t/t_display_cwide_bad.v:10:20: $display-like format of %c format of > 8 bit value + : ... note: In instance 't' 10 | $display("%c", 32'h1234); - | ^~~~~~~~ + | ^~~~~~~~ ... For warning description see https://verilator.org/warn/WIDTHTRUNC?v=latest ... Use "/* verilator lint_off WIDTHTRUNC */" and lint_on around source to disable this message. %Error: Exiting due to diff --git a/test_regress/t/t_display_type_bad.out b/test_regress/t/t_display_type_bad.out index 6f4040efe..50603e15e 100644 --- a/test_regress/t/t_display_type_bad.out +++ b/test_regress/t/t_display_type_bad.out @@ -1,20 +1,20 @@ -%Error: t/t_display_type_bad.v:11:29: $display-line format of '%d' illegal with string argument +%Error: t/t_display_type_bad.v:11:29: $display-like format of '%d' illegal with non-numeric argument : ... note: In instance 't' : ... Suggest use '%s' 11 | $display("%d %x %f %t", s, s, s, s); | ^ ... See the manual at https://verilator.org/verilator_doc.html?v=latest for more assistance. -%Error: t/t_display_type_bad.v:11:32: $display-line format of '%x' illegal with string argument +%Error: t/t_display_type_bad.v:11:32: $display-like format of '%x' illegal with non-numeric argument : ... note: In instance 't' : ... Suggest use '%s' 11 | $display("%d %x %f %t", s, s, s, s); | ^ -%Error: t/t_display_type_bad.v:11:35: $display-line format of '%f' illegal with string argument +%Error: t/t_display_type_bad.v:11:35: $display-like format of '%f' illegal with non-numeric argument : ... note: In instance 't' : ... Suggest use '%s' 11 | $display("%d %x %f %t", s, s, s, s); | ^ -%Error: t/t_display_type_bad.v:11:38: $display-line format of '%t' illegal with string argument +%Error: t/t_display_type_bad.v:11:38: $display-like format of '%t' illegal with non-numeric argument : ... note: In instance 't' : ... Suggest use '%s' 11 | $display("%d %x %f %t", s, s, s, s);