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

@@ -1135,49 +1135,33 @@ void AstNode::v3errorEndFatal(std::ostringstream& str) const {
     VL_UNREACHABLE
 }
 
-string AstNode::locationStr() const {
+string AstNode::instanceStr() const {
-    string str = "... In instance ";
+    // Max iterations before giving up on location search,
-    const AstNode* backp = this;
+    // in case we have some circular reference bug.
-    int itmax = 10000;  // Max iterations before giving up on location search
+    constexpr unsigned maxIterations = 10000;
-    while (backp) {
+    unsigned iterCount = 0;
-        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;
 
-            str += scopep->prettyName();
+    for (const AstNode* backp = this; backp; backp = backp->backp(), ++iterCount) {
-            return str;
+        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();
+
-    }
+        // If scopes don't exist, report an example instance of the enclosing module
-    backp = this;
+        if (const AstModule* const modp = VN_CAST(backp, Module)) {
-    while (backp) {
+            const string instanceName = modp->someInstanceName();
-        const AstModule* modp;
+            return instanceName.empty() ? "" : "... In instance " + instanceName;
-        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;
-            }
         }
-        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<AstNode*>(this)->dump(nsstr);
             nsstr << endl;
         }
-        m_fileline->v3errorEnd(nsstr, locationStr());
+        m_fileline->v3errorEnd(nsstr, instanceStr());
     }
 }
 

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; }
+    string someInstanceName() const { return m_someInstanceName; }
-    void hierName(const string& hierName) { m_hierName = hierName; }
+    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; }

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()) {
+    if (!extra.empty()) {
-        const string locationMsg = warnMore() + locationStr + "\n";
+        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 (

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.

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) {

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

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 {

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);
                         }
                     }
                 }

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

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