From d04647f4598642d011dab462fffb8d6d8c4e1eb4 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Sat, 5 Sep 2026 19:48:07 +0100 Subject: [PATCH] Fix library method argument references (#8252) Each VCMethod now carries a signature describing the access required of the references passed as arguments to the call. 'r' if the argument is read, 'w' if it is fully assigned so the old value does not matter, 'm' if it is modified (or only conditionally assigned), with a trailing '+' repeating the preceding entry for all remaining arguments. Signatures are validated at compile time, and V3Broken checks the arguments of every AstCMethodHard against them. The incorrect references this found and that are easy to fix are repaired in this patch. "TODO" marks method that are currently broken or not yet fit the scheme, these will be fixed in follow up patches. Also renames SCHED_COMMIT and SCHED_ENQUEUE to NBA_COMMIT and NBA_ENQUEUE, and remove unused methods. --- include/verilated_force.h | 2 +- src/V3AssertPre.cpp | 1 + src/V3AstAttr.h | 278 +++++++++++++++++++++----------------- src/V3AstNodeExpr.h | 11 +- src/V3AstNodes.cpp | 113 ++++++++++------ src/V3Broken.cpp | 47 +++++++ src/V3Delayed.cpp | 5 +- src/V3Force.cpp | 20 +-- src/V3FsmDetect.cpp | 8 +- src/V3LinkDot.cpp | 2 +- src/V3Unknown.cpp | 4 +- 11 files changed, 304 insertions(+), 187 deletions(-) diff --git a/include/verilated_force.h b/include/verilated_force.h index 5cbafdc62..8b3155540 100644 --- a/include/verilated_force.h +++ b/include/verilated_force.h @@ -303,7 +303,7 @@ public: } template - T readIndex(T origVal, int index) const { + T readIndex(const T origVal, int index) const { if (m_entries.empty()) return origVal; T result = origVal; diff --git a/src/V3AssertPre.cpp b/src/V3AssertPre.cpp index a7a2ca464..2b40ca537 100644 --- a/src/V3AssertPre.cpp +++ b/src/V3AssertPre.cpp @@ -414,6 +414,7 @@ private: flp, new AstVarRef{flp, queueVarp, VAccess::READWRITE}, VCMethod::DYN_POP, new AstTime{nodep->fileline(), m_modp->timeunit()}}; popp->addPinsp(skewp->unlinkFrBack()); + refp->access(VAccess::READWRITE); // Only conditionally assigned popp->addPinsp(refp); popp->dtypeSetVoid(); m_clockingp->addNextHere( diff --git a/src/V3AstAttr.h b/src/V3AstAttr.h index 1cb4a960e..be33e611b 100644 --- a/src/V3AstAttr.h +++ b/src/V3AstAttr.h @@ -812,132 +812,137 @@ inline std::ostream& operator<<(std::ostream& os, const VBranchPred& rhs) { // ###################################################################### -// C++ methods invoked on runtime library data types via AstCMethodHard +// C++ methods invoked on runtime library data types via AstCMethodHard. +// The argument descriptor gives the access direction of each argument: +// - 'r' read by the call +// - 'w' written by the call (Use only if unconditionally and wholly written, +// that is: a preceding write can be removed. Otherwise use 'm'.) +// - 'm' read and written (modified) by the call +// - '+' repeats the entry before it for all remaining arguments, must be last +// - "" if the method takes no arguments +// - "TODO" if not yet checked due to existing issues // clang-format off #define FOR_EACH_CMETHOD(macro) \ - /* id, method, pure */ \ - macro(_NONE, "_none", false) \ - macro(ARRAY_AND, "and", true) \ - macro(ARRAY_AT, "at", true) \ - macro(ARRAY_AT_BACK, "atBack", true) \ - macro(ARRAY_AT_WRITE, "atWrite", true) \ - macro(ARRAY_FIND, "find", true) \ - macro(ARRAY_FIND_FIRST, "find_first", true) \ - macro(ARRAY_FIND_FIRST_INDEX, "find_first_index", true) \ - macro(ARRAY_FIND_INDEX, "find_index", true) \ - macro(ARRAY_FIND_LAST, "find_last", true) \ - macro(ARRAY_FIND_LAST_INDEX, "find_last_index", true) \ - macro(ARRAY_FIRST, "first", false) \ - macro(ARRAY_INSIDE, "inside", true) \ - macro(ARRAY_LAST, "last", false) \ - macro(ARRAY_MAP, "map", true) \ - macro(ARRAY_MAX, "max", true) \ - macro(ARRAY_MIN, "min", true) \ - macro(ARRAY_NEXT, "next", false) \ - macro(ARRAY_OR, "or", true) \ - macro(ARRAY_POP_BACK, "pop_back", false) \ - macro(ARRAY_POP_FRONT, "pop_front", false) \ - macro(ARRAY_PREV, "prev", false) \ - macro(ARRAY_PRODUCT, "product", true) \ - macro(ARRAY_PUSH_BACK, "push_back", false) \ - macro(ARRAY_PUSH_FRONT, "push_front", false) \ - macro(ARRAY_REVERSE, "reverse", false) \ - macro(ARRAY_RSORT, "rsort", false) \ - macro(ARRAY_R_AND, "r_and", true) \ - macro(ARRAY_R_OR, "r_or", true) \ - macro(ARRAY_R_PRODUCT, "r_product", true) \ - macro(ARRAY_R_SUM, "r_sum", true) \ - macro(ARRAY_R_XOR, "r_xor", true) \ - macro(ARRAY_SHUFFLE, "shuffle", false) \ - macro(ARRAY_SORT, "sort", false) \ - macro(ARRAY_SUM, "sum", true) \ - macro(ARRAY_UNIQUE, "unique", true) \ - macro(ARRAY_UNIQUE_INDEX, "unique_index", true) \ - macro(ARRAY_XOR, "xor", true) \ - macro(ASSOC_CLEAR, "clear", false) \ - macro(ASSOC_ERASE, "erase", false) \ - macro(ASSOC_EXISTS, "exists", true) \ - macro(ASSOC_FIRST, "first", false) \ - macro(ASSOC_NEXT, "next", false) \ - macro(ASSOC_SIZE, "size", true) \ - macro(CLASS_SET_RANDMODE, "set_randmode", false) \ - macro(DYN_AT_WRITE_APPEND, "atWriteAppend", false) \ - macro(DYN_AT_WRITE_APPEND_BACK, "atWriteAppendBack", false) \ - macro(DYN_CLEAR, "clear", false) \ - macro(DYN_ERASE, "erase", false) \ - macro(DYN_INSERT, "insert", false) \ - macro(DYN_POP, "pop", false) \ - macro(DYN_POP_FRONT, "pop_front", false) \ - macro(DYN_PUSH, "push", false) \ - macro(DYN_PUSH_FRONT, "push_front", false) \ - macro(DYN_RENEW, "renew", false) \ - macro(DYN_RENEW_COPY, "renew_copy", false) \ - macro(DYN_RESIZE, "resize", false) \ - macro(DYN_SIZE, "size", true) \ - macro(DYN_SLICE, "slice", true) \ - macro(DYN_SLICE_ASSIGN, "sliceAssign", false) \ - macro(DYN_SLICE_ASSIGN_BACK_BACK, "sliceAssignBackBack", false) \ - macro(DYN_SLICE_ASSIGN_FRONT_BACK, "sliceAssignFrontBack", false) \ - macro(DYN_SLICE_BACK_BACK, "sliceBackBack", true) \ - macro(DYN_SLICE_FRONT_BACK, "sliceFrontBack", true) \ - macro(EVENT_CLEAR_FIRED, "clearFired", false) \ - macro(EVENT_CLEAR_TRIGGERED, "clearTriggered", false) \ - macro(EVENT_FIRE, "fire", false) \ - macro(EVENT_IS_FIRED, "isFired", true) \ - macro(EVENT_IS_TRIGGERED, "isTriggered", true) \ - macro(FORCE_ADD, "addForce", false) \ - macro(FORCE_READ, "read", true) \ - macro(FORCE_READ_INDEX, "readIndex", true) \ - macro(FORCE_READ_SEL, "readSel", true) \ - macro(FORCE_RELEASE, "release", false) \ - macro(FORCE_TOUCH, "touch", false) \ - macro(FORK_DONE, "done", false) \ - macro(FORK_INIT, "init", false) \ - macro(FORK_JOIN, "join", false) \ - macro(FORK_ON_KILL, "onKill", false) \ - macro(RANDOMIZER_BASIC_STD_RANDOMIZATION, "basicStdRandomization", false) \ - macro(RANDOMIZER_CLEARCONSTRAINTS, "clearConstraints", false) \ - macro(RANDOMIZER_CLEARALL, "clearAll", false) \ - macro(RANDOMIZER_DISABLE_SOFT, "disable_soft", false) \ - macro(RANDOMIZER_HARD, "hard", false) \ - macro(RANDOMIZER_SOFT, "soft", false) \ - macro(RANDOMIZER_UNIQUE, "rand_unique", false) \ - macro(RANDOMIZER_MARK_RANDC, "markRandc", false) \ - macro(RANDOMIZER_SOLVE_BEFORE, "solveBefore", false) \ - macro(RANDOMIZER_PIN_VAR, "pin_var", false) \ - macro(RANDOMIZER_WRITE_VAR, "write_var", false) \ - macro(RANDOMIZER_SET_VAR_DISABLED, "set_var_disabled", false) \ - macro(RANDOMIZER_CLEAR_VAR_DISABLED, "clear_var_disabled", false) \ - macro(RANDOMIZER_MARK_VAR_STATIC, "mark_var_static", false) \ - macro(RANDOMIZER_SET_STATIC_RANDMODE, "set_static_randmode", false) \ - macro(RNG_GET_RANDSTATE, "__Vm_rng.get_randstate", true) \ - macro(RNG_SET_RANDSTATE, "__Vm_rng.set_randstate", false) \ - macro(SCHED_ANY_TRIGGERED, "anyTriggered", false) \ - macro(SCHED_AWAITING_CURRENT_TIME, "awaitingCurrentTime", true) \ - macro(SCHED_AWAITING_ZERO_DELAY, "awaitingZeroDelay", true) \ - macro(SCHED_READY, "ready", false) \ - macro(SCHED_COMMIT, "commit", false) \ - macro(SCHED_MOVE_TO_RESUME_QUEUE, "moveToResumeQueue", false) \ - macro(SCHED_DELAY, "delay", false) \ - macro(SCHED_DO_POST_UPDATES, "doPostUpdates", false) \ - macro(SCHED_ENQUEUE, "enqueue", false) \ - macro(SCHED_EVALUATE, "evaluate", false) \ - macro(SCHED_EVALUATION, "evaluation", false) \ - macro(SCHED_POST_UPDATE, "postUpdate", false) \ - macro(SCHED_RESUME, "resume", false) \ - macro(SCHED_RESUME_ZERO_DELAY, "resumeZeroDelay", false) \ - macro(SCHED_RESUMPTION, "resumption", false) \ - macro(SCHED_TRIGGER, "trigger", false) \ - macro(SCHED_WAIT_FOREVER, "waitForever", false) \ - macro(UNPACKED_ASSIGN, "assign", false) \ - macro(UNPACKED_FILL, "fill", false) \ - macro(UNPACKED_NEQ, "neq", true) + /* id, method, pure, args */ \ + macro(_NONE, "_none", false, "") \ + macro(ARRAY_AT, "at", PURE, "r") \ + macro(ARRAY_AT_BACK, "atBack", PURE, "r") \ + macro(ARRAY_AT_WRITE, "atWrite", PURE, "r") \ + macro(ARRAY_FIND, "find", PURE, "") \ + macro(ARRAY_FIND_FIRST, "find_first", PURE, "") \ + macro(ARRAY_FIND_FIRST_INDEX, "find_first_index", PURE, "") \ + macro(ARRAY_FIND_INDEX, "find_index", PURE, "") \ + macro(ARRAY_FIND_LAST, "find_last", PURE, "") \ + macro(ARRAY_FIND_LAST_INDEX, "find_last_index", PURE, "") \ + macro(ARRAY_FIRST, "first", false, "m") \ + macro(ARRAY_INSIDE, "inside", PURE, "r") \ + macro(ARRAY_LAST, "last", false, "m") \ + macro(ARRAY_MAP, "map", PURE, "") \ + macro(ARRAY_MAX, "max", PURE, "") \ + macro(ARRAY_MIN, "min", PURE, "") \ + macro(ARRAY_NEXT, "next", false, "m") \ + macro(ARRAY_POP_BACK, "pop_back", false, "") \ + macro(ARRAY_POP_FRONT, "pop_front", false, "") \ + macro(ARRAY_PREV, "prev", false, "m") \ + macro(ARRAY_PUSH_BACK, "push_back", false, "r") \ + macro(ARRAY_PUSH_FRONT, "push_front", false, "r") \ + macro(ARRAY_REVERSE, "reverse", false, "") \ + macro(ARRAY_RSORT, "rsort", false, "") \ + macro(ARRAY_R_AND, "r_and", PURE, "") \ + macro(ARRAY_R_OR, "r_or", PURE, "") \ + macro(ARRAY_R_PRODUCT, "r_product", PURE, "") \ + macro(ARRAY_R_SUM, "r_sum", PURE, "") \ + macro(ARRAY_R_XOR, "r_xor", PURE, "") \ + macro(ARRAY_SHUFFLE, "shuffle", false, "") \ + macro(ARRAY_SORT, "sort", false, "") \ + macro(ARRAY_UNIQUE, "unique", PURE, "") \ + macro(ARRAY_UNIQUE_INDEX, "unique_index", PURE, "") \ + macro(ASSOC_CLEAR, "clear", false, "") \ + macro(ASSOC_ERASE, "erase", false, "r") \ + macro(ASSOC_EXISTS, "exists", PURE, "r") \ + macro(ASSOC_FIRST, "first", false, "m") \ + macro(ASSOC_NEXT, "next", false, "m") \ + macro(ASSOC_SIZE, "size", PURE, "") \ + macro(CLASS_SET_RANDMODE, "set_randmode", false, "r") \ + macro(DYN_AT_WRITE_APPEND, "atWriteAppend", false, "r") \ + macro(DYN_AT_WRITE_APPEND_BACK, "atWriteAppendBack", false, "r") \ + macro(DYN_CLEAR, "clear", false, "") \ + macro(DYN_ERASE, "erase", false, "r") \ + macro(DYN_INSERT, "insert", false, "rr") \ + macro(DYN_POP, "pop", false, "rrm") \ + macro(DYN_POP_FRONT, "pop_front", false, "") \ + macro(DYN_PUSH, "push", false, "rr") \ + macro(DYN_PUSH_FRONT, "push_front", false, "r") \ + macro(DYN_RENEW, "renew", false, "r") \ + macro(DYN_RENEW_COPY, "renew_copy", false, "rr") \ + macro(DYN_RESIZE, "resize", false, "TODO") \ + macro(DYN_SIZE, "size", PURE, "") \ + macro(DYN_SLICE, "slice", PURE, "rr") \ + macro(DYN_SLICE_ASSIGN, "sliceAssign", false, "rrr") \ + macro(DYN_SLICE_ASSIGN_BACK_BACK, "sliceAssignBackBack", false, "rrr") \ + macro(DYN_SLICE_ASSIGN_FRONT_BACK, "sliceAssignFrontBack", false, "rrr") \ + macro(DYN_SLICE_BACK_BACK, "sliceBackBack", PURE, "rr") \ + macro(DYN_SLICE_FRONT_BACK, "sliceFrontBack", PURE, "rr") \ + macro(EVENT_CLEAR_FIRED, "clearFired", false, "") \ + macro(EVENT_CLEAR_TRIGGERED, "clearTriggered", false, "") \ + macro(EVENT_FIRE, "fire", false, "") \ + macro(EVENT_IS_FIRED, "isFired", PURE, "") \ + macro(EVENT_IS_TRIGGERED, "isTriggered", PURE, "") \ + macro(FORCE_ADD, "addForce", false, "rrrr+") \ + macro(FORCE_READ, "read", PURE, "r") \ + macro(FORCE_READ_INDEX, "readIndex", PURE, "rr") \ + macro(FORCE_READ_SEL, "readSel", PURE, "TODO") \ + macro(FORCE_RELEASE, "release", false, "rr+") \ + macro(FORCE_TOUCH, "touch", false, "") \ + macro(FORK_DONE, "done", false, "rr") \ + macro(FORK_INIT, "init", false, "rr") \ + macro(FORK_JOIN, "join", false, "rrr") \ + macro(FORK_ON_KILL, "onKill", false, "r") \ + macro(NBA_COMMIT, "commit", false, "w") \ + macro(NBA_ENQUEUE, "enqueue", false, "r+") \ + macro(RANDOMIZER_BASIC_STD_RANDOMIZATION, "basicStdRandomization", false, "mr") \ + macro(RANDOMIZER_CLEARCONSTRAINTS, "clearConstraints", false, "") \ + macro(RANDOMIZER_CLEARALL, "clearAll", false, "") \ + macro(RANDOMIZER_DISABLE_SOFT, "disable_soft", false, "r") \ + macro(RANDOMIZER_HARD, "hard", false, "r+") \ + macro(RANDOMIZER_SOFT, "soft", false, "rrrr") \ + macro(RANDOMIZER_UNIQUE, "rand_unique", false, "r") \ + macro(RANDOMIZER_MARK_RANDC, "markRandc", false, "r") \ + macro(RANDOMIZER_SOLVE_BEFORE, "solveBefore", false, "rr") \ + macro(RANDOMIZER_PIN_VAR, "pin_var", false, "rrr") \ + macro(RANDOMIZER_WRITE_VAR, "write_var", false, "TODO") \ + macro(RANDOMIZER_SET_VAR_DISABLED, "set_var_disabled", false, "r") \ + macro(RANDOMIZER_CLEAR_VAR_DISABLED, "clear_var_disabled", false, "r") \ + macro(RANDOMIZER_MARK_VAR_STATIC, "mark_var_static", false, "r") \ + macro(RANDOMIZER_SET_STATIC_RANDMODE, "set_static_randmode", false, "r") \ + macro(RNG_GET_RANDSTATE, "__Vm_rng.get_randstate", PURE, "") \ + macro(RNG_SET_RANDSTATE, "__Vm_rng.set_randstate", false, "r") \ + macro(SCHED_ANY_TRIGGERED, "anyTriggered", false, "r") \ + macro(SCHED_AWAITING_CURRENT_TIME, "awaitingCurrentTime", PURE, "") \ + macro(SCHED_AWAITING_ZERO_DELAY, "awaitingZeroDelay", PURE, "") \ + macro(SCHED_READY, "ready", false, "r") \ + macro(SCHED_MOVE_TO_RESUME_QUEUE, "moveToResumeQueue", false, "r") \ + macro(SCHED_DELAY, "delay", false, "rrrr") \ + macro(SCHED_DO_POST_UPDATES, "doPostUpdates", false, "") \ + macro(SCHED_EVALUATE, "evaluate", false, "") \ + macro(SCHED_EVALUATION, "evaluation", false, "rrrr") \ + macro(SCHED_POST_UPDATE, "postUpdate", false, "rrrr") \ + macro(SCHED_RESUME, "resume", false, "TODO") \ + macro(SCHED_RESUME_ZERO_DELAY, "resumeZeroDelay", false, "") \ + macro(SCHED_RESUMPTION, "resumption", false, "rrrr") \ + macro(SCHED_TRIGGER, "trigger", false, "rrrrr") \ + macro(SCHED_WAIT_FOREVER, "waitForever", false, "rrr") \ + macro(UNPACKED_ASSIGN, "assign", false, "r") \ + macro(UNPACKED_FILL, "fill", false, "r") \ + macro(UNPACKED_NEQ, "neq", PURE, "r") // clang-format on class VCMethod final { + static constexpr bool PURE = true; // For macro expansion of 'pure' field only + public: enum en : uint8_t { -#define VL_CMETHOD_ID(id, method, pure) id, +#define VL_CMETHOD_ID(id, method, pure, args) id, FOR_EACH_CMETHOD(VL_CMETHOD_ID) #undef VL_CMETHOD_ID _ENUM_MAX // Leave last @@ -953,7 +958,7 @@ public: constexpr operator en() const { return m_e; } const char* ascii() const VL_PURE { static const char* const values[] = { -#define VL_CMETHOD_NAME(id, method, pure) method, +#define VL_CMETHOD_NAME(id, method, pure, args) method, FOR_EACH_CMETHOD(VL_CMETHOD_NAME) #undef VL_CMETHOD_NAME "_ENUM_MAX" // @@ -962,15 +967,39 @@ public: } bool isPure() const VL_PURE { static const bool values[] = { -#define VL_CMETHOD_PURE(id, method, pure) pure, +#define VL_CMETHOD_PURE(id, method, pure, args) pure, FOR_EACH_CMETHOD(VL_CMETHOD_PURE) #undef VL_CMETHOD_PURE false // }; return values[m_e]; } + const char* args() const VL_PURE { + static const char* const values[] = { +#define VL_CMETHOD_ARGS(id, method, pure, args) args, + FOR_EACH_CMETHOD(VL_CMETHOD_ARGS) +#undef VL_CMETHOD_ARGS + "" // + }; + return values[m_e]; + } // Return array method for given name static VCMethod arrayMethod(const string& name); + + // Validate the arguments descriptor + static constexpr bool validateArgsDescriptor(const char* descrp) { + // Is "TODO" + if (descrp[0] == 'T' && descrp[1] == 'O' && descrp[2] == 'D' && descrp[3] == 'O' + && !descrp[4]) { + return true; + } + // Is a sequence of 'r'/'w'/'m' with an optional trailing '+' + for (const char* cp = descrp; *cp; ++cp) { + if (*cp == '+') return cp != descrp && !cp[1]; + if (*cp != 'r' && *cp != 'w' && *cp != 'm') return false; + } + return true; + } }; constexpr bool operator==(const VCMethod& lhs, const VCMethod& rhs) { return lhs.m_e == rhs.m_e; } constexpr bool operator==(const VCMethod& lhs, VCMethod::en rhs) { return lhs.m_e == rhs; } @@ -979,6 +1008,13 @@ inline std::ostream& operator<<(std::ostream& os, const VCMethod& rhs) { return os << rhs.ascii(); } +// Static assert all argument descriptors are well formed +#define VL_CMETHOD_ARGS_CHECK(id, method, pure, args) \ + static_assert(VCMethod::validateArgsDescriptor(args), \ + "Malformed argument descriptor for " #id); +FOR_EACH_CMETHOD(VL_CMETHOD_ARGS_CHECK) +#undef VL_CMETHOD_ARGS_CHECK + #undef FOR_EACH_CMETHOD // ###################################################################### diff --git a/src/V3AstNodeExpr.h b/src/V3AstNodeExpr.h index 3e4c6e21c..a1b2b8b6c 100644 --- a/src/V3AstNodeExpr.h +++ b/src/V3AstNodeExpr.h @@ -66,7 +66,15 @@ public: bool isOpaque() const { return VN_IS(this, CvtPackString); } // True for SVA multi-cycle sequence nodes (SExpr, SConsRep, etc.) virtual bool isMultiCycleSva() const { return false; } + + // TODO: consolidate cLValueTargetp, isLValue, baseFromp + // If the expression is a valid C++ LValue, return the target reference, else nullptr + // This always returns either AstVarRef, AstMemberSel, or nullptr + AstNodeExpr* cLValueTargetp(); + // TODO: this actually means it's a write or RW, not that it's an LValue bool isLValue() const; + // Return base var (or const) nodep dereferences + AstNode* baseFromp(bool overMembers); // Wrap This expression into an AstStmtExpr to denote it occurs in statement position inline AstStmtExpr* makeStmt(); @@ -4922,9 +4930,6 @@ public: bool isPredictOptimizable() const override { return true; } bool sameNode(const AstNode* /*samep*/) const override { return true; } int instrCount() const override { return widthInstrs(); } - // Special operators - // Return base var (or const) nodep dereferences - static AstNode* baseFromp(AstNode* nodep, bool overMembers); }; class AstAssocSel final : public AstNodeSel { void init(const AstNode* fromp) { diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index 70b43ee8e..5f2b92da5 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -266,7 +266,7 @@ bool AstNode::isDisableQueuePushSelfStmt() { if (!stmtExprp) return false; AstCMethodHard* const methodp = VN_CAST(stmtExprp->exprp(), CMethodHard); if (!methodp || methodp->name() != "push_back") return false; - AstNode* const basep = AstArraySel::baseFromp(methodp->fromp(), false); + AstNode* const basep = methodp->fromp()->baseFromp(false); if (AstVarRef* const refp = VN_CAST(basep, VarRef)) return refp->varp()->processQueue(); if (AstMemberSel* const selp = VN_CAST(basep, MemberSel)) { return selp->varp() && selp->varp()->processQueue(); @@ -314,50 +314,6 @@ const char* AstAnd::widthMismatch() const VL_MT_STABLE { BROKEN_RTN(lhsp()->widthMin() != widthMin()); return nullptr; } -/// What is the base variable (or const) this dereferences? -AstNode* AstArraySel::baseFromp(AstNode* nodep, bool overMembers) { - // Else AstArraySel etc; search for the base - while (nodep) { - if (VN_IS(nodep, ArraySel)) { - nodep = VN_AS(nodep, ArraySel)->fromp(); - continue; - } else if (VN_IS(nodep, Sel)) { - nodep = VN_AS(nodep, Sel)->fromp(); - continue; - } else if (VN_IS(nodep, AssocSel)) { - nodep = VN_AS(nodep, AssocSel)->fromp(); - continue; - } else if (VN_IS(nodep, WildcardSel)) { - nodep = VN_AS(nodep, WildcardSel)->fromp(); - continue; - } else if (VN_IS(nodep, CMethodHard)) { - nodep = VN_AS(nodep, CMethodHard)->fromp(); - continue; - } else if (overMembers && VN_IS(nodep, MemberSel)) { - nodep = VN_AS(nodep, MemberSel)->fromp(); - continue; - } else if (overMembers && VN_IS(nodep, StructSel)) { - nodep = VN_AS(nodep, StructSel)->fromp(); - continue; - } - // AstNodePreSel stashes the associated variable under an ATTROF - // of VAttrType::VAR_BASE so it isn't constified - else if (VN_IS(nodep, AttrOf)) { - nodep = VN_AS(nodep, AttrOf)->fromp(); - continue; - } else if (VN_IS(nodep, NodePreSel)) { - if (VN_AS(nodep, NodePreSel)->attrp()) { - nodep = VN_AS(nodep, NodePreSel)->attrp(); - } else { - nodep = VN_AS(nodep, NodePreSel)->fromp(); - } - continue; - } else { - break; - } - } - return nodep; -} AstAssertCtl::AstAssertCtl(FileLine* fl, VAssertCtlType ctlType, uint32_t assertType, uint32_t directiveType, AstNodeExpr* levelp, AstNodeExpr* itemsp) : ASTGEN_SUPER_AssertCtl(fl) @@ -2514,6 +2470,73 @@ int AstNodeDType::widthStream() const { } return dtypep->width(); } +// What is the base variable (or const) this dereferences? +AstNode* AstNodeExpr::baseFromp(bool overMembers) { + AstNode* nodep = this; + while (nodep) { + if (VN_IS(nodep, ArraySel)) { + nodep = VN_AS(nodep, ArraySel)->fromp(); + continue; + } else if (VN_IS(nodep, Sel)) { + nodep = VN_AS(nodep, Sel)->fromp(); + continue; + } else if (VN_IS(nodep, AssocSel)) { + nodep = VN_AS(nodep, AssocSel)->fromp(); + continue; + } else if (VN_IS(nodep, WildcardSel)) { + nodep = VN_AS(nodep, WildcardSel)->fromp(); + continue; + } else if (VN_IS(nodep, CMethodHard)) { + nodep = VN_AS(nodep, CMethodHard)->fromp(); + continue; + } else if (overMembers && VN_IS(nodep, MemberSel)) { + nodep = VN_AS(nodep, MemberSel)->fromp(); + continue; + } else if (overMembers && VN_IS(nodep, StructSel)) { + nodep = VN_AS(nodep, StructSel)->fromp(); + continue; + } + // AstNodePreSel stashes the associated variable under an ATTROF + // of VAttrType::VAR_BASE so it isn't constified + else if (VN_IS(nodep, AttrOf)) { + nodep = VN_AS(nodep, AttrOf)->fromp(); + continue; + } else if (VN_IS(nodep, NodePreSel)) { + if (VN_AS(nodep, NodePreSel)->attrp()) { + nodep = VN_AS(nodep, NodePreSel)->attrp(); + } else { + nodep = VN_AS(nodep, NodePreSel)->fromp(); + } + continue; + } else { + break; + } + } + return nodep; +} +AstNodeExpr* AstNodeExpr::cLValueTargetp() { + // Leaves + if (AstVarRef* const refp = VN_CAST(this, VarRef)) { // + return refp; + } + if (AstMemberSel* const selp = VN_CAST(this, MemberSel)) { // + return selp; + } + + // Recursive + if (AstSel* const selp = VN_CAST(this, Sel)) { // + return selp->fromp()->cLValueTargetp(); + } + if (AstStructSel* const selp = VN_CAST(this, StructSel)) { // + return selp->fromp()->cLValueTargetp(); + } + if (AstNodeSel* const selp = VN_CAST(this, NodeSel)) { // Array, Assoc, Wildcard, Word + return selp->fromp()->cLValueTargetp(); + } + + // Not an LValue + return nullptr; +} void AstNodeExpr::dump(std::ostream& str) const { Super::dump(str); } void AstNodeExpr::dumpJson(std::ostream& str) const { dumpJsonGen(str); } bool AstNodeExpr::isLValue() const { diff --git a/src/V3Broken.cpp b/src/V3Broken.cpp index ba5aa0135..863f2751e 100644 --- a/src/V3Broken.cpp +++ b/src/V3Broken.cpp @@ -227,6 +227,52 @@ private: } return false; } + static void checkArgRefs(AstNodeExpr* nodep, const char* descrp, AstNodeExpr* argsp) { + if (!std::strcmp(descrp, "TODO")) return; // Skip if not yet checked + + // Check each argument + const char* dp = descrp; + for (AstNodeExpr* argp = argsp; argp; argp = VN_AS(argp->nextp(), NodeExpr)) { + if (argp->fileline()->erroringOn()) return; // Intentionally skip all checks + const AstNodeExpr* const lvalp = argp->cLValueTargetp(); + const VAccess access = [&]() -> VAccess { + if (const AstVarRef* const varrefp = VN_CAST(lvalp, VarRef)) { + return varrefp->access(); + } + if (const AstMemberSel* const memberselp = VN_CAST(lvalp, MemberSel)) { + return memberselp->access(); + } + UASSERT_OBJ(!lvalp, argp, "Unknown LValue expression"); + // Not an LValue, so it's read-only + return VAccess::READ; + }(); + if (dp[0] == '+') --dp; // Repeats the entry before it + switch (dp[0]) { + case 'r': + UASSERT_OBJ(access.isReadOnly(), argp, + "Input argument of library call is not a read-only expression"); + break; + case 'w': + UASSERT_OBJ(lvalp, argp, // + "Output argument of library call is not a valid C++ LValue"); + UASSERT_OBJ(access.isWriteOnly(), argp, + "Output argument of library call is not write-only"); + break; + case 'm': + UASSERT_OBJ(lvalp, argp, // + "Inout argument of library call is not a valid C++ LValue"); + UASSERT_OBJ(access.isRW(), argp, + "Inout argument of library call is not read-write"); + break; + default: + UASSERT_OBJ(dp[0], argp, "Unexpected trailing arguments to library call"); + break; // LCOV_EXCL_LINE + } + ++dp; + } + UASSERT_OBJ(!dp[0] || dp[0] == '+', nodep, "Insufficient arguments to library call"); + } + // VISITORS void visit(AstNodeAssign* nodep) override { processEnter(nodep); @@ -291,6 +337,7 @@ private: void visit(AstCMethodHard* nodep) override { ++m_nCalls; processAndIterate(nodep); + checkArgRefs(nodep, nodep->method().args(), nodep->pinsp()); } void visit(AstNodeFTaskRef* nodep) override { ++m_nCalls; diff --git a/src/V3Delayed.cpp b/src/V3Delayed.cpp index f2ed71ebc..ef3c0154b 100644 --- a/src/V3Delayed.cpp +++ b/src/V3Delayed.cpp @@ -865,8 +865,9 @@ class DelayedVisitor final : public VNVisitor { activep->addStmtsp(postp); // Add the commit AstCMethodHard* const callp = new AstCMethodHard{ - flp, new AstVarRef{flp, queueVscp, VAccess::READWRITE}, VCMethod::SCHED_COMMIT}; + flp, new AstVarRef{flp, queueVscp, VAccess::READWRITE}, VCMethod::NBA_COMMIT}; callp->dtypeSetVoid(); + // TODO: this is a partial update, so must be READWRITE, but that breaks scheduling callp->addPinsp(new AstVarRef{flp, vscp, VAccess::WRITE}); postp->addStmtsp(callp->makeStmt()); } @@ -975,7 +976,7 @@ class DelayedVisitor final : public VNVisitor { // Enqueue the update at the site of the original NBA AstCMethodHard* const callp = new AstCMethodHard{ flp, new AstVarRef{flp, vscpInfo.valueQueueKit().vscp, VAccess::READWRITE}, - VCMethod::SCHED_ENQUEUE}; + VCMethod::NBA_ENQUEUE}; callp->dtypeSetVoid(); callp->addPinsp(valuep); if (partial) callp->addPinsp(maskp); diff --git a/src/V3Force.cpp b/src/V3Force.cpp index edc9dd705..47f078418 100644 --- a/src/V3Force.cpp +++ b/src/V3Force.cpp @@ -268,7 +268,7 @@ public: } static AstVarRef* getOneVarRef(AstNodeExpr* forceStmtp) { - AstNode* const basep = AstArraySel::baseFromp(forceStmtp, true); + AstNode* const basep = forceStmtp->baseFromp(true); if (AstSampled* sampledp = VN_CAST(basep, Sampled)) if (AstNodeExpr* exprp = VN_CAST(sampledp->exprp(), NodeExpr)) return getOneVarRef(exprp); @@ -822,8 +822,9 @@ public: AstNodeExpr* createForceReadExpression(const VarForceInfo& varInfo, AstVarRef* originalRefp) const { FileLine* const flp = originalRefp->fileline(); - return createForceReadCall(varInfo, flp, VCMethod::FORCE_READ, - originalRefp->cloneTreePure(false), originalRefp->varp(), + AstVarRef* const refp = originalRefp->cloneTreePure(false); + refp->access(VAccess::READ); + return createForceReadCall(varInfo, flp, VCMethod::FORCE_READ, refp, refp->varp(), nullptr); } @@ -831,8 +832,11 @@ public: AstNodeExpr* originalExprp, AstNodeExpr* indexExprp) const { FileLine* const flp = originalExprp->fileline(); - return createForceReadCall(varInfo, flp, VCMethod::FORCE_READ_INDEX, - originalExprp->cloneTreePure(false), originalExprp, indexExprp); + AstNodeExpr* const exprp = originalExprp->cloneTreePure(false); + // Must be an LValue to a static variable + VN_AS(exprp->cLValueTargetp(), VarRef)->access(VAccess::READ); + return createForceReadCall(varInfo, flp, VCMethod::FORCE_READ_INDEX, exprp, originalExprp, + indexExprp); } static AstNodeExpr* rebuildSelPath(AstNodeExpr* pathp, AstNodeExpr* baseExprp) { @@ -1315,7 +1319,7 @@ class ForceReplaceVisitor final : public VNVisitor { m_stmtp = nodep; iterate(nodep->lhsp()); iterate(nodep->rhsp()); - if (AstVarRef* const lhsp = VN_CAST(AstArraySel::baseFromp(nodep->lhsp(), true), VarRef)) { + if (AstVarRef* const lhsp = VN_CAST(nodep->lhsp()->baseFromp(true), VarRef)) { if (AstNode* const updatep = m_state.createRhsUpdatesForWrite(nodep->fileline(), lhsp->varp())) { nodep->addNextHere(updatep); @@ -1394,7 +1398,7 @@ class ForceReplaceVisitor final : public VNVisitor { } } - AstNode* const basep = AstArraySel::baseFromp(nodep, true); + AstNode* const basep = nodep->baseFromp(true); AstVarRef* const baseRefp = VN_CAST(basep, VarRef); if (!baseRefp) { iterateChildren(nodep); @@ -1492,7 +1496,7 @@ class ForceReplaceVisitor final : public VNVisitor { // Handle the whole opaque path at its outermost node so we can assign one stable // synthetic force-path index to the full selection/member chain. AstNodeExpr* const exprp = VN_AS(nodep, NodeExpr); - AstNode* const basep = AstArraySel::baseFromp(exprp, true); + AstNode* const basep = exprp->baseFromp(true); AstVarRef* const baseRefp = VN_CAST(basep, VarRef); if (baseRefp) { AstVar* const varp = baseRefp->varp(); diff --git a/src/V3FsmDetect.cpp b/src/V3FsmDetect.cpp index 526867913..90e990e9f 100644 --- a/src/V3FsmDetect.cpp +++ b/src/V3FsmDetect.cpp @@ -994,7 +994,7 @@ class FsmDetectVisitor final : public VNVisitor { AstVarScope*& fromVscp) { AstNodeAssign* const assp = VN_CAST(nodep, NodeAssign); if (!assp) return nullptr; - AstVarRef* const lhsp = VN_CAST(AstArraySel::baseFromp(assp->lhsp(), true), VarRef); + AstVarRef* const lhsp = VN_CAST(assp->lhsp()->baseFromp(true), VarRef); AstVarRef* const rhsp = VN_CAST(assp->rhsp(), VarRef); if (!rhsp || !lhsp) return nullptr; stateVscp = lhsp->varScopep(); @@ -1008,7 +1008,7 @@ class FsmDetectVisitor final : public VNVisitor { FsmStateValue& resetValue) { AstNodeAssign* const assp = VN_CAST(nodep, NodeAssign); if (!assp) return nullptr; - AstVarRef* const lhsp = VN_CAST(AstArraySel::baseFromp(assp->lhsp(), true), VarRef); + AstVarRef* const lhsp = VN_CAST(assp->lhsp()->baseFromp(true), VarRef); AstCond* const rhsp = VN_CAST(assp->rhsp(), Cond); if (!rhsp || !lhsp) return nullptr; if (AstVarRef* const elsep = VN_CAST(rhsp->elsep(), VarRef)) { @@ -1033,7 +1033,7 @@ class FsmDetectVisitor final : public VNVisitor { FsmStateValue& value) { AstNodeAssign* const assp = VN_CAST(nodep, NodeAssign); if (!assp) return nullptr; - AstVarRef* const lhsp = VN_CAST(AstArraySel::baseFromp(assp->lhsp(), true), VarRef); + AstVarRef* const lhsp = VN_CAST(assp->lhsp()->baseFromp(true), VarRef); UASSERT_OBJ(lhsp, assp, "direct constant state assignment lhs should be normalized to a VarRef"); if (constValueStatus(assp->rhsp(), value) != ConstValueStatus::OK) return nullptr; @@ -1220,7 +1220,7 @@ class FsmDetectVisitor final : public VNVisitor { AstVarRef* vrefp = VN_CAST(eqp->lhsp(), VarRef); AstNodeExpr* valuep = eqp->rhsp(); if (!vrefp) { - vrefp = VN_CAST(AstArraySel::baseFromp(eqp->rhsp(), true), VarRef); + vrefp = VN_CAST(eqp->rhsp()->baseFromp(true), VarRef); if (!vrefp) { return false; } valuep = eqp->lhsp(); } diff --git a/src/V3LinkDot.cpp b/src/V3LinkDot.cpp index 5d1283764..a25aa44a1 100644 --- a/src/V3LinkDot.cpp +++ b/src/V3LinkDot.cpp @@ -5572,7 +5572,7 @@ class LinkDotResolveVisitor final : public VNVisitor { AstNode* const attrp = nodep->attrp()->unlinkFrBack(); VL_DO_DANGLING(attrp->deleteTree(), attrp); } - AstNode* const basefromp = AstArraySel::baseFromp(nodep, false); + AstNode* const basefromp = nodep->baseFromp(false); if (VN_IS(basefromp, Replicate)) { // From {...}[...] syntax in IEEE 2017 if (basefromp) UINFO(9, indent() << " Related node: " << basefromp); diff --git a/src/V3Unknown.cpp b/src/V3Unknown.cpp index 57e12e478..2fdc04394 100644 --- a/src/V3Unknown.cpp +++ b/src/V3Unknown.cpp @@ -396,7 +396,7 @@ class UnknownVisitor final : public VNVisitor { iterateChildren(nodep); if (!nodep->user1SetOnce()) { // Guard against reading/writing past end of bit vector array - const AstNode* const basefromp = AstArraySel::baseFromp(nodep, true); + const AstNode* const basefromp = nodep->baseFromp(true); bool lvalue = false; if (const AstNodeVarRef* const varrefp = VN_CAST(basefromp, NodeVarRef)) { lvalue = varrefp->access().isWriteOrRW(); @@ -455,7 +455,7 @@ class UnknownVisitor final : public VNVisitor { if (!nodep->user1SetOnce()) { UINFOTREE(9, nodep, "", "in"); // Guard against reading/writing past end of arrays - AstNode* const basefromp = AstArraySel::baseFromp(nodep->fromp(), true); + AstNode* const basefromp = nodep->fromp()->baseFromp(true); bool lvalue = false; if (const AstNodeVarRef* const varrefp = VN_CAST(basefromp, NodeVarRef)) { lvalue = varrefp->access().isWriteOrRW();