diff --git a/ci/ci-build.bash b/ci/ci-build.bash index add5624d2..f5e384950 100755 --- a/ci/ci-build.bash +++ b/ci/ci-build.bash @@ -69,6 +69,9 @@ fi if [ "$OPT_ASAN" = 1 ]; then CONFIGURE_ARGS="$CONFIGURE_ARGS --enable-dev-asan" CXX="$CXX -DVL_LEAK_CHECKS" + # Replaces VNUser's union with a std::variant to validate type usage. + # Only takes effect when the build is C++17 or newer. + CXX="$CXX -DVL_USER_TYPE_CHECKS" fi if [ "$OPT_GCOV" = 1 ]; then CONFIGURE_ARGS="$CONFIGURE_ARGS --enable-dev-gcov" diff --git a/src/Makefile_obj.in b/src/Makefile_obj.in index 1c2b08a73..1697fcfe4 100644 --- a/src/Makefile_obj.in +++ b/src/Makefile_obj.in @@ -105,6 +105,9 @@ CPPFLAGS += -I. -I$(bldsrc) -I$(srcdir) -I$(incdir) -I../../include #CPPFLAGS += -DVL_ALLOC_RANDOM_CHECKS # To allow --debug-new-random #CPPFLAGS += -DVL_LEAK_CHECKS # If running valgrind or other hunting tool CPPFLAGS += -MP # Only works on recent GCC versions +# Replaces VNUser's union with a std::variant to validate int vs void* usage +# Ignored unless the build is C++17 or newer +#CPPFLAGS += -DVL_USER_TYPE_CHECKS ifeq ($(CFG_WITH_CCWARN),yes) # Local... Else don't burden users CPPFLAGS += -W -Wall $(CFG_CXXFLAGS_WEXTRA) $(CFG_CXXFLAGS_SRC) -Werror #CPPFLAGS += -pedantic-errors diff --git a/src/V3Ast.cpp b/src/V3Ast.cpp index 112482bd8..20f536bb1 100644 --- a/src/V3Ast.cpp +++ b/src/V3Ast.cpp @@ -71,6 +71,21 @@ void VCMethod::selfTest() { } } +//###################################################################### +// VNUser + +std::string VNUser::dumpStr(std::string (*fmtAddrp)(const void*)) const { +#ifdef VL_USER_TYPE_CHECKS + if (const int* const uip = std::get_if(&m_u)) return "#"s + cvtToStr(*uip); + if (void* const* const upp = std::get_if(&m_u)) return fmtAddrp(*upp); + return ""; +#else + // Dumps void* representation + if (!m_u.up) return ""; + return fmtAddrp(m_u.up); +#endif +} + //###################################################################### // VNType @@ -1384,10 +1399,15 @@ void AstNode::dumpPtrs(std::ostream& os) const { if (op2p()) os << " op2p=" << cvtToHex(op2p()); if (op3p()) os << " op3p=" << cvtToHex(op3p()); if (op4p()) os << " op4p=" << cvtToHex(op4p()); - if (user1p()) os << " user1p=" << cvtToHex(user1p()); - if (user2p()) os << " user2p=" << cvtToHex(user2p()); - if (user3p()) os << " user3p=" << cvtToHex(user3p()); - if (user4p()) os << " user4p=" << cvtToHex(user4p()); + const auto dumpUser = [&os](const char* prefix, const VNUser& user) { + const std::string s + = user.dumpStr([](const void* p) -> std::string { return cvtToHex(p); }); + if (!s.empty()) os << prefix << s; + }; + dumpUser(" user1p=", user1u()); + dumpUser(" user2p=", user2u()); + dumpUser(" user3p=", user3u()); + dumpUser(" user4p=", user4u()); if (m_iterpp) { os << " iterpp=" << cvtToHex(m_iterpp); // This may cause address sanitizer failures as iterpp can be stale diff --git a/src/V3Ast.h b/src/V3Ast.h index 404995928..4807930c3 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -40,6 +40,14 @@ #include #include +// VL_USER_TYPE_CHECKS requires C++17 for std::variant. Ignore on older standards +#if defined(VL_USER_TYPE_CHECKS) && __cplusplus < 201703L +#undef VL_USER_TYPE_CHECKS +#endif +#ifdef VL_USER_TYPE_CHECKS +#include +#endif + // clang-format off #include "V3AstAttr.h" // clang-format on @@ -137,12 +145,48 @@ class V3GraphVertex; class VSymEnt; class VNUser final { +// Defining VL_USER_TYPE_CHECKS replaces the VNUser union with a std::variant +// which remembers which form of VNUser is being stored to catch mismatched reads. +#ifdef VL_USER_TYPE_CHECKS + // monostate is an unwritten / cleared slot. It can be read as either form + // and yields nullptr/0. + std::variant m_u; +#else union { void* up; int ui; } m_u; +#endif public: +#ifdef VL_USER_TYPE_CHECKS + VNUser() = default; + // non-explicit: + // cppcheck-suppress noExplicitConstructor + VNUser(int i) { + // VNUser{0} represents the monostate + if (i) m_u = i; + } + explicit VNUser(void* p) { + // VNUser{nullptr} represents the monostate + if (p) m_u = p; + } + ~VNUser() = default; + // Casters + template + typename std::enable_if::value, T>::type to() const VL_MT_SAFE { + if (std::holds_alternative(m_u)) return nullptr; + void* const* const upp = std::get_if(&m_u); + UASSERT_STATIC(upp, "AstNode user() slot written as int, read as pointer"); + return reinterpret_cast(*upp); + } + int toInt() const { + if (std::holds_alternative(m_u)) return 0; + const int* const uip = std::get_if(&m_u); + UASSERT_STATIC(uip, "AstNode user() slot written as pointer, read as int"); + return *uip; + } +#else VNUser() = default; // non-explicit: // cppcheck-suppress noExplicitConstructor @@ -157,10 +201,14 @@ public: typename std::enable_if::value, T>::type to() const VL_MT_SAFE { return reinterpret_cast(m_u.up); } + int toInt() const { return m_u.ui; } +#endif VSymEnt* toSymEnt() const { return to(); } AstNode* toNodep() const VL_MT_SAFE { return to(); } V3GraphVertex* toGraphVertex() const { return to(); } - int toInt() const { return m_u.ui; } + // Render for dumps without asserting on the form held: "" if unset, "#" + // if an int, else the pointer via fmtAddrp + std::string dumpStr(std::string (*fmtAddrp)(const void*)) const; }; //###################################################################### @@ -441,6 +489,8 @@ class AstNode VL_NOT_FINAL { static int s_cloneCntGbl; // Count of which userp is set // This member ordering both allows 64 bit alignment and puts associated data together + // (under VL_USER_TYPE_CHECKS a VNUser is larger than 64 bits, so this packing no + // longer holds; that build trades node size for catching int/pointer confusion) VNUser m_user1u{0}; // Contains any information the user iteration routine wants uint32_t m_user1Cnt = 0; // Mark of when userp was set uint32_t m_user2Cnt = 0; // Mark of when userp was set diff --git a/src/V3AstNodes.cpp b/src/V3AstNodes.cpp index c01f657c4..e607ce289 100644 --- a/src/V3AstNodes.cpp +++ b/src/V3AstNodes.cpp @@ -1846,10 +1846,16 @@ void AstNode::dump(std::ostream& str) const { #endif << " {" << fileline()->filenameLetters() << std::dec << fileline()->lastLineno() << fileline()->firstColumnLetters() << "}"; - if (user1p()) str << " u1=" << nodeAddr(user1p()); - if (user2p()) str << " u2=" << nodeAddr(user2p()); - if (user3p()) str << " u3=" << nodeAddr(user3p()); - if (user4p()) str << " u4=" << nodeAddr(user4p()); + const auto dumpUser = [&str](const char* prefix, const VNUser& user) { + const std::string s = user.dumpStr([](const void* p) -> std::string { + return nodeAddr(reinterpret_cast(p)); + }); + if (!s.empty()) str << prefix << s; + }; + dumpUser(" u1=", user1u()); + dumpUser(" u2=", user2u()); + dumpUser(" u3=", user3u()); + dumpUser(" u4=", user4u()); if (hasDType()) { // Final @ so less likely to by accident read it as a nodep if (dtypep() == this) {