From 5f0e1fae7fe5bebefd4e95da3e18f0a2de1c8d41 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Fri, 22 Apr 2022 22:39:45 +0100 Subject: [PATCH] Simplify and clarify reporting of enclosing instance Rename AstNodeModule::hierName -> someInstanceName and explain that this is only used for user messages. Rename AstNode::locationStr -> instanceStr and simplify implementation. In particular, do not report an instance if we can't find a reasonable guess. --- src/V3Ast.cpp | 56 +++++++++++------------------- src/V3Ast.h | 9 ++--- src/V3Error.cpp | 8 ++--- src/V3Error.h | 2 +- src/V3FileLine.cpp | 6 ++-- src/V3FileLine.h | 2 +- src/V3Hasher.cpp | 3 +- src/V3Param.cpp | 19 +++++----- test_regress/t/t_force_bad_rw.out | 1 - test_regress/t/t_fuzz_eqne_bad.out | 1 - 10 files changed, 46 insertions(+), 61 deletions(-) diff --git a/src/V3Ast.cpp b/src/V3Ast.cpp index 5a18c98ee..1bd2cc14e 100644 --- a/src/V3Ast.cpp +++ b/src/V3Ast.cpp @@ -1135,49 +1135,33 @@ void AstNode::v3errorEndFatal(std::ostringstream& str) const { VL_UNREACHABLE } -string AstNode::locationStr() const { - string str = "... In instance "; - const AstNode* backp = this; - int itmax = 10000; // Max iterations before giving up on location search - while (backp) { - if (VL_UNCOVERABLE(--itmax < 0)) { - // Likely some circular back link, and V3Ast is trying to report a low-level error - UINFO(1, "Ran out of iterations finding locationStr on " << backp << endl); - return ""; // LCOV_EXCL_LINE - } - const AstScope* scopep; - if ((scopep = VN_CAST(backp, Scope))) { - // The design is flattened and there are no useful scopes - // This is probably because of inlining - if (scopep->isTop()) break; +string AstNode::instanceStr() const { + // Max iterations before giving up on location search, + // in case we have some circular reference bug. + constexpr unsigned maxIterations = 10000; + unsigned iterCount = 0; - str += scopep->prettyName(); - return str; + for (const AstNode* backp = this; backp; backp = backp->backp(), ++iterCount) { + if (VL_UNCOVERABLE(iterCount >= maxIterations)) return ""; // LCOV_EXCL_LINE + + // Prefer the enclosing scope, if there is one. This is always under the enclosing module, + // so just pick it up when encountered + if (const AstScope* const scopep = VN_CAST(backp, Scope)) { + return scopep->isTop() ? "" : "... In instance " + scopep->prettyName(); } - backp = backp->backp(); - } - backp = this; - while (backp) { - const AstModule* modp; - const AstNodeVarRef* nvrp; - if ((modp = VN_CAST(backp, Module)) && !modp->hierName().empty()) { - str += modp->hierName(); - return str; - } else if ((nvrp = VN_CAST(backp, NodeVarRef))) { - const string prettyName = nvrp->prettyName(); - // VarRefs have not been flattened yet and do not contain location information - if (prettyName != nvrp->name()) { - str += prettyName; - return str; - } + + // If scopes don't exist, report an example instance of the enclosing module + if (const AstModule* const modp = VN_CAST(backp, Module)) { + const string instanceName = modp->someInstanceName(); + return instanceName.empty() ? "" : "... In instance " + instanceName; } - backp = backp->backp(); } + return ""; } void AstNode::v3errorEnd(std::ostringstream& str) const { if (!m_fileline) { - V3Error::v3errorEnd(str, locationStr()); + V3Error::v3errorEnd(str, instanceStr()); } else { std::ostringstream nsstr; nsstr << str.str(); @@ -1187,7 +1171,7 @@ void AstNode::v3errorEnd(std::ostringstream& str) const { const_cast(this)->dump(nsstr); nsstr << endl; } - m_fileline->v3errorEnd(nsstr, locationStr()); + m_fileline->v3errorEnd(nsstr, instanceStr()); } } diff --git a/src/V3Ast.h b/src/V3Ast.h index 1e717b6e5..9ddf48e43 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -1447,7 +1447,7 @@ private: bool gateOnly); void deleteTreeIter(); void deleteNode(); - string locationStr() const; + string instanceStr() const; public: static void relinkOneLink(AstNode*& pointpr, AstNode* newp); @@ -3077,7 +3077,8 @@ class AstNodeModule VL_NOT_FINAL : public AstNode { private: string m_name; // Name of the module const string m_origName; // Name of the module, ignoring name() changes, for dot lookup - string m_hierName; // Hierarchical name for errors, etc. + string m_someInstanceName; // Hierarchical name of some arbitrary instance of this module. + // Used for user messages only. bool m_modPublic : 1; // Module has public references bool m_modTrace : 1; // Tracing this module bool m_inLibrary : 1; // From a library, no error if not used, never top level @@ -3119,8 +3120,8 @@ public: // ACCESSORS virtual void name(const string& name) override { m_name = name; } virtual string origName() const override { return m_origName; } - string hierName() const { return m_hierName; } - void hierName(const string& hierName) { m_hierName = hierName; } + string someInstanceName() const { return m_someInstanceName; } + void someInstanceName(const string& name) { m_someInstanceName = name; } bool inLibrary() const { return m_inLibrary; } void inLibrary(bool flag) { m_inLibrary = flag; } void level(int level) { m_level = level; } diff --git a/src/V3Error.cpp b/src/V3Error.cpp index 9452c9ff8..1f1f8d08f 100644 --- a/src/V3Error.cpp +++ b/src/V3Error.cpp @@ -183,7 +183,7 @@ void V3Error::suppressThisWarning() { string V3Error::warnMore() { return string(msgPrefix().size(), ' '); } -void V3Error::v3errorEnd(std::ostringstream& sstr, const string& locationStr) { +void V3Error::v3errorEnd(std::ostringstream& sstr, const string& extra) { #if defined(__COVERITY__) || defined(__cppcheck__) if (s_errorCode == V3ErrorCode::EC_FATAL) __coverity_panic__(x); #endif @@ -209,10 +209,10 @@ void V3Error::v3errorEnd(std::ostringstream& sstr, const string& locationStr) { // Suppress duplicate messages if (s_messages.find(msg) != s_messages.end()) return; s_messages.insert(msg); - if (!locationStr.empty()) { - const string locationMsg = warnMore() + locationStr + "\n"; + if (!extra.empty()) { + const string extraMsg = warnMore() + extra + "\n"; const size_t pos = msg.find('\n'); - msg.insert(pos + 1, locationMsg); + msg.insert(pos + 1, extraMsg); } // Output if ( diff --git a/src/V3Error.h b/src/V3Error.h index b0b4663d7..077260995 100644 --- a/src/V3Error.h +++ b/src/V3Error.h @@ -313,7 +313,7 @@ public: static void vlAbortOrExit(); static void vlAbort(); // static, but often overridden in classes. - static void v3errorEnd(std::ostringstream& sstr, const string& locationStr = ""); + static void v3errorEnd(std::ostringstream& sstr, const string& extra = ""); }; // Global versions, so that if the class doesn't define a operator, we get the functions anyways. diff --git a/src/V3FileLine.cpp b/src/V3FileLine.cpp index 29d99d7c4..fa5e324fe 100644 --- a/src/V3FileLine.cpp +++ b/src/V3FileLine.cpp @@ -340,15 +340,15 @@ void FileLine::modifyStateInherit(const FileLine* fromp) { } } -void FileLine::v3errorEnd(std::ostringstream& sstr, const string& locationStr) { +void FileLine::v3errorEnd(std::ostringstream& sstr, const string& extra) { std::ostringstream nsstr; if (lastLineno()) nsstr << this; nsstr << sstr.str(); nsstr << endl; std::ostringstream lstr; - if (!locationStr.empty()) { + if (!extra.empty()) { lstr << std::setw(ascii().length()) << " " - << ": " << locationStr; + << ": " << extra; } m_waive = V3Config::waive(this, V3Error::errorCode(), sstr.str()); if (warnIsOff(V3Error::errorCode()) || m_waive) { diff --git a/src/V3FileLine.h b/src/V3FileLine.h index dbc437588..1928e8f5a 100644 --- a/src/V3FileLine.h +++ b/src/V3FileLine.h @@ -245,7 +245,7 @@ public: void modifyWarnOff(V3ErrorCode code, bool flag) { warnOff(code, flag); } // OPERATORS - void v3errorEnd(std::ostringstream& str, const string& locationStr = ""); + void v3errorEnd(std::ostringstream& str, const string& extra = ""); void v3errorEndFatal(std::ostringstream& str); /// When building an error, prefix for printing continuation lines /// e.g. information referring to the same FileLine as before diff --git a/src/V3Hasher.cpp b/src/V3Hasher.cpp index 56652e647..10c53d4c8 100644 --- a/src/V3Hasher.cpp +++ b/src/V3Hasher.cpp @@ -299,9 +299,8 @@ private: m_hash += hashNodeAndIterate(nodep, HASH_DTYPE, HASH_CHILDREN, [=]() {}); } virtual void visit(AstNodeModule* nodep) override { - m_hash += hashNodeAndIterate(nodep, HASH_DTYPE, false, [=]() { + m_hash += hashNodeAndIterate(nodep, HASH_DTYPE, false, [=]() { // m_hash += nodep->origName(); - m_hash += nodep->hierName(); }); } virtual void visit(AstNodePreSel* nodep) override { diff --git a/src/V3Param.cpp b/src/V3Param.cpp index ea1936ae3..ebd55b526 100644 --- a/src/V3Param.cpp +++ b/src/V3Param.cpp @@ -761,7 +761,7 @@ class ParamProcessor final { } public: - void cellDeparam(AstCell* nodep, AstNodeModule* modp, const string& hierName) { + void cellDeparam(AstCell* nodep, AstNodeModule* modp, const string& someInstanceName) { m_modp = modp; // Cell: Check for parameters in the instantiation. // We always run this, even if no parameters, as need to look for interfaces, @@ -772,7 +772,7 @@ public: // Evaluate all module constants V3Const::constifyParamsEdit(nodep); AstNodeModule* const srcModp = nodep->modp(); - srcModp->hierName(hierName + "." + nodep->name()); + srcModp->someInstanceName(someInstanceName + "." + nodep->name()); // Make sure constification worked // Must be a separate loop, as constant conversion may have changed some pointers. @@ -857,11 +857,11 @@ class ParamVisitor final : public VNVisitor { // METHODS VL_DEBUG_FUNC; // Declare debug() - void visitCellDeparam(AstCell* nodep, const string& hierName) { + void visitCellDeparam(AstCell* nodep, const string& someInstanceName) { // Cell: Check for parameters in the instantiation. iterateChildren(nodep); UASSERT_OBJ(nodep->modp(), nodep, "Not linked?"); - m_processor.cellDeparam(nodep, m_modp, hierName); + m_processor.cellDeparam(nodep, m_modp, someInstanceName); // Remember to process the child module at the end of the module m_todoModps.emplace(nodep->modp()->level(), nodep->modp()); } @@ -877,8 +877,11 @@ class ParamVisitor final : public VNVisitor { // again m_modp = nodep; UINFO(4, " MOD " << nodep << endl); - if (m_modp->hierName().empty()) m_modp->hierName(m_modp->origName()); + if (m_modp->someInstanceName().empty()) { + m_modp->someInstanceName(m_modp->origName()); + } iterateChildren(nodep); + // Note above iterate may add to m_todoModps // // Process interface cells, then non-interface which may ref an interface cell @@ -886,13 +889,13 @@ class ParamVisitor final : public VNVisitor { for (AstCell* const cellp : m_cellps) { if ((nonIf == 0 && VN_IS(cellp->modp(), Iface)) || (nonIf == 1 && !VN_IS(cellp->modp(), Iface))) { - string fullName(m_modp->hierName()); + string someInstanceName(m_modp->someInstanceName()); if (const string* const genHierNamep = (string*)cellp->user5p()) { - fullName += *genHierNamep; + someInstanceName += *genHierNamep; cellp->user5p(nullptr); VL_DO_DANGLING(delete genHierNamep, genHierNamep); } - VL_DO_DANGLING(visitCellDeparam(cellp, fullName), cellp); + VL_DO_DANGLING(visitCellDeparam(cellp, someInstanceName), cellp); } } } diff --git a/test_regress/t/t_force_bad_rw.out b/test_regress/t/t_force_bad_rw.out index caa8482e2..0b287ff80 100644 --- a/test_regress/t/t_force_bad_rw.out +++ b/test_regress/t/t_force_bad_rw.out @@ -1,5 +1,4 @@ %Error: t/t_force_bad_rw.v:14:20: Unsupported: Signals used via read-write reference cannot be forced - : ... In instance t.unnamedblk1.unnamedblk1.index 14 | foreach (ass[index]) begin | ^~~~~ %Error: Exiting due to diff --git a/test_regress/t/t_fuzz_eqne_bad.out b/test_regress/t/t_fuzz_eqne_bad.out index 08bd37daf..a04d877ab 100644 --- a/test_regress/t/t_fuzz_eqne_bad.out +++ b/test_regress/t/t_fuzz_eqne_bad.out @@ -1,5 +1,4 @@ %Error: t/t_fuzz_eqne_bad.v:12:23: Slice operator VARREF 't.b' on non-slicable (e.g. non-vector) right-hand-side operand - : ... In instance t.b 12 | initial c = (a != &b); | ^ %Error: Exiting due to