From 0ca90257a52d756806d8719015de0dea0356d364 Mon Sep 17 00:00:00 2001 From: Wilson Snyder Date: Sat, 6 Jan 2024 16:14:58 -0500 Subject: [PATCH] Add predicted stack overflow warning (#4799). --- Changes | 1 + include/verilated.cpp | 28 +++++ include/verilated.h | 1 + src/CMakeLists.txt | 4 +- src/Makefile_obj.in | 1 + src/V3EmitCSyms.cpp | 10 ++ src/V3InstrCount.cpp | 10 +- src/V3Options.cpp | 1 + src/V3Options.h | 2 + src/V3StackCount.cpp | 202 ++++++++++++++++++++++++++++++++ src/V3StackCount.h | 38 ++++++ test_regress/t/t_stack_check.pl | 24 ++++ test_regress/t/t_stack_check.v | 14 +++ 13 files changed, 330 insertions(+), 6 deletions(-) create mode 100644 src/V3StackCount.cpp create mode 100644 src/V3StackCount.h create mode 100755 test_regress/t/t_stack_check.pl create mode 100644 test_regress/t/t_stack_check.v diff --git a/Changes b/Changes index 414a560c3..c06d4479f 100644 --- a/Changes +++ b/Changes @@ -13,6 +13,7 @@ Verilator 5.021 devel **Minor:** +* Add predicted stack overflow warning (#4799). * Remove deprecated 32-bit pointer mode (`gcc -m32`). * Fix delays using wrong timeunits when modules inlined (#4806). [Paul Wright] diff --git a/include/verilated.cpp b/include/verilated.cpp index 4631e8644..1a32e2b1d 100644 --- a/include/verilated.cpp +++ b/include/verilated.cpp @@ -71,6 +71,11 @@ # include # define _VL_HAVE_STACKTRACE #endif +#if defined(__linux) || (defined(__APPLE__) && defined(__MACH__)) +# include +# include +# define _VL_HAVE_GETRLIMIT +#endif #include "verilated_threads.h" // clang-format on @@ -2995,6 +3000,29 @@ void Verilated::scTraceBeforeElaborationError() VL_MT_SAFE { VL_UNREACHABLE; } +void Verilated::stackCheck(QData needSize) VL_MT_UNSAFE { + // Slowpath - Called only when constructing +#ifdef _VL_HAVE_GETRLIMIT + QData haveSize = 0; + rlimit rlim; + if (0 == getrlimit(RLIMIT_STACK, &rlim)) { + haveSize = rlim.rlim_cur; + if (haveSize == RLIM_INFINITY) haveSize = rlim.rlim_max; + if (haveSize == RLIM_INFINITY) haveSize = 0; + } + // VL_PRINTF_MT("-Info: stackCheck(%" PRIu64 ") have %" PRIu64 "\n", needSize, haveSize); + // Check for 1.5x need, but suggest 2x so small model increase won't cause warning + // if the user follows the suggestions + if (VL_UNLIKELY(haveSize && needSize && haveSize < (needSize + needSize / 2))) { + VL_PRINTF_MT("%%Warning: System has stack size %" PRIu64 " kb" + " which may be too small; suggest at least 'ulimit -c %" PRIu64 "'\n", + haveSize / 1024, (needSize * 2) / 1024); + } +#else + if (false && needSize) {} // Unused argument +#endif +} + void Verilated::mkdir(const char* dirname) VL_MT_UNSAFE { #if defined(_WIN32) || defined(__MINGW32__) ::mkdir(dirname); diff --git a/include/verilated.h b/include/verilated.h index 14aa42e99..89181ace1 100644 --- a/include/verilated.h +++ b/include/verilated.h @@ -898,6 +898,7 @@ public: static void overWidthError(const char* signame) VL_ATTR_NORETURN VL_MT_SAFE; static void scTimePrecisionError(int sc_prec, int vl_prec) VL_ATTR_NORETURN VL_MT_SAFE; static void scTraceBeforeElaborationError() VL_ATTR_NORETURN VL_MT_SAFE; + static void stackCheck(QData needSize) VL_MT_UNSAFE; // Internal: Get and set DPI context static const VerilatedScope* dpiScope() VL_MT_SAFE { return t_s.t_dpiScopep; } diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 61c05a21d..db970c21a 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -149,6 +149,7 @@ set(HEADERS V3Split.h V3SplitAs.h V3SplitVar.h + V3StackCount.h V3Stats.h V3StdFuture.h V3String.h @@ -258,8 +259,8 @@ set(COMMON_SOURCES V3LinkDot.cpp V3LinkInc.cpp V3LinkJump.cpp - V3LinkLevel.cpp V3LinkLValue.cpp + V3LinkLevel.cpp V3LinkParse.cpp V3LinkResolve.cpp V3Localize.cpp @@ -289,6 +290,7 @@ set(COMMON_SOURCES V3Split.cpp V3SplitAs.cpp V3SplitVar.cpp + V3StackCount.cpp V3Stats.cpp V3StatsReport.cpp V3String.cpp diff --git a/src/Makefile_obj.in b/src/Makefile_obj.in index 857ce1c7b..99b284757 100644 --- a/src/Makefile_obj.in +++ b/src/Makefile_obj.in @@ -281,6 +281,7 @@ RAW_OBJS_PCH_ASTNOMT = \ V3Split.o \ V3SplitAs.o \ V3SplitVar.o \ + V3StackCount.o \ V3Subst.o \ V3TSP.o \ V3Table.o \ diff --git a/src/V3EmitCSyms.cpp b/src/V3EmitCSyms.cpp index 8ddd82a6e..217645f73 100644 --- a/src/V3EmitCSyms.cpp +++ b/src/V3EmitCSyms.cpp @@ -20,6 +20,8 @@ #include "V3EmitCBase.h" #include "V3LanguageWords.h" #include "V3PartitionGraph.h" +#include "V3StackCount.h" +#include "V3Stats.h" #include #include @@ -810,6 +812,14 @@ void EmitCSyms::emitSymImp() { } puts("{\n"); + { + puts(" // Check resources\n"); + uint64_t stackSize = V3StackCount::count(v3Global.rootp()); + if (v3Global.opt.debugStackCheck()) stackSize += 1024 * 1024 * 1024; + V3Stats::addStat("Stack size prediction (bytes)", stackSize); + puts(" Verilated::stackCheck(" + cvtToStr(stackSize) + ");\n"); + } + if (v3Global.opt.profPgo()) { puts("// Configure profiling for PGO\n"); if (v3Global.opt.mtasks()) { diff --git a/src/V3InstrCount.cpp b/src/V3InstrCount.cpp index 704df1abc..71a05e976 100644 --- a/src/V3InstrCount.cpp +++ b/src/V3InstrCount.cpp @@ -71,7 +71,7 @@ public: : m_startNodep{nodep} , m_assertNoDups{assertNoDups} , m_osp{osp} { - if (nodep) iterateConst(nodep); + iterateConstNull(nodep); } ~InstrCountVisitor() override = default; @@ -208,12 +208,12 @@ private: const uint32_t elseCount = m_instrCount; reset(); - if (ifCount < elseCount) { - m_instrCount = savedCount + elseCount; - if (nodep->thenp()) nodep->thenp()->user2(0); // Don't dump it - } else { + if (ifCount >= elseCount) { m_instrCount = savedCount + ifCount; if (nodep->elsep()) nodep->elsep()->user2(0); // Don't dump it + } else { + m_instrCount = savedCount + elseCount; + if (nodep->thenp()) nodep->thenp()->user2(0); // Don't dump it } } void visit(AstCAwait* nodep) override { diff --git a/src/V3Options.cpp b/src/V3Options.cpp index 7a72a8dc5..340f4c48f 100644 --- a/src/V3Options.cpp +++ b/src/V3Options.cpp @@ -1166,6 +1166,7 @@ void V3Options::parseOptsList(FileLine* fl, const string& optdir, int argc, DECL_OPTION("-debug-protect", OnOff, &m_debugProtect).undocumented(); DECL_OPTION("-debug-self-test", OnOff, &m_debugSelfTest).undocumented(); DECL_OPTION("-debug-sigsegv", CbCall, throwSigsegv).undocumented(); // See also --debug-abort + DECL_OPTION("-debug-stack-check", OnOff, &m_debugStackCheck).undocumented(); DECL_OPTION("-decoration", OnOff, &m_decoration); DECL_OPTION("-dpi-hdr-only", OnOff, &m_dpiHdrOnly); DECL_OPTION("-dump-", CbPartialMatch, [this](const char* optp) { m_dumpLevel[optp] = 3; }); diff --git a/src/V3Options.h b/src/V3Options.h index bed53babe..5db70ffe6 100644 --- a/src/V3Options.h +++ b/src/V3Options.h @@ -243,6 +243,7 @@ private: bool m_debugPartition = false; // main switch: --debug-partition bool m_debugProtect = false; // main switch: --debug-protect bool m_debugSelfTest = false; // main switch: --debug-self-test + bool m_debugStackCheck = false; // main switch: --debug-stack-check bool m_decoration = true; // main switch: --decoration bool m_dpiHdrOnly = false; // main switch: --dpi-hdr-only bool m_exe = false; // main switch: --exe @@ -471,6 +472,7 @@ public: bool debugPartition() const { return m_debugPartition; } bool debugProtect() const VL_MT_SAFE { return m_debugProtect; } bool debugSelfTest() const { return m_debugSelfTest; } + bool debugStackCheck() const { return m_debugStackCheck; } bool decoration() const VL_MT_SAFE { return m_decoration; } bool dpiHdrOnly() const { return m_dpiHdrOnly; } bool dumpDefines() const { return m_dumpLevel.count("defines") && m_dumpLevel.at("defines"); } diff --git a/src/V3StackCount.cpp b/src/V3StackCount.cpp new file mode 100644 index 000000000..f500b8602 --- /dev/null +++ b/src/V3StackCount.cpp @@ -0,0 +1,202 @@ +// -*- mode: C++; c-file-style: "cc-mode" -*- +//************************************************************************* +// DESCRIPTION: Verilator: Estimate stack size to run the AST subtree. +// +// Code available from: https://verilator.org +// +//************************************************************************* +// +// Copyright 2003-2024 by Wilson Snyder. This program is free software; you +// can redistribute it and/or modify it under the terms of either the GNU +// Lesser General Public License Version 3 or the Perl Artistic License +// Version 2.0. +// SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 +// +//************************************************************************* + +#include "V3PchAstNoMT.h" // VL_MT_DISABLED_CODE_UNIT + +#include "V3StackCount.h" + +VL_DEFINE_DEBUG_FUNCTIONS; + +/// Estimate the stack byte size for executing all logic within and below a +/// given AST node. This is very rough, only for warning when stack is too +/// small. + +class StackCountVisitor final : public VNVisitorConst { + // NODE STATE + // AstNode::user2() -> int. Path cost + 1, + const VNUser2InUse m_inuser2; + + // MEMBERS + uint64_t m_stackSize = 0; // Running count of instructions + bool m_tracingCall = false; // Iterating into a CCall to a CFunc + bool m_ignoreRemaining = false; // Ignore remaining statements in the block + bool m_inCFunc = false; // Inside function + + // TYPES + // Little class to cleanly call startVisitBase/endVisitBase + class VisitBase final { + // MEMBERS + uint32_t m_savedCount; + AstNode* const m_nodep; + StackCountVisitor* const m_visitor; + + public: + // CONSTRUCTORS + VisitBase(StackCountVisitor* visitor, AstNode* nodep) + : m_nodep{nodep} + , m_visitor{visitor} { + m_savedCount = m_visitor->startVisitBase(nodep); + } + ~VisitBase() { m_visitor->endVisitBase(m_savedCount, m_nodep); } + + private: + VL_UNCOPYABLE(VisitBase); + }; + +public: + // CONSTRUCTORS + StackCountVisitor(AstNode* nodep) { iterateConstNull(nodep); } + ~StackCountVisitor() override = default; + + // METHODS + uint32_t stackSize() const { return m_stackSize; } + +private: + void reset() { + m_stackSize = 0; + m_ignoreRemaining = false; + } + uint32_t startVisitBase(AstNode* nodep) { + UASSERT_OBJ(!m_ignoreRemaining, nodep, "Should not reach here if ignoring"); + + // Save the count, and add it back in during ~VisitBase This allows + // debug prints to show local cost of each subtree, so we can see a + // hierarchical view of the cost when in debug mode. + const uint32_t savedCount = m_stackSize; + m_stackSize = 0; + return savedCount; + } + void endVisitBase(uint32_t savedCount, AstNode* nodep) { + UINFO(8, "cost " << std::setw(6) << std::left << m_stackSize << " " << nodep << endl); + if (!m_ignoreRemaining) m_stackSize += savedCount; + } + + // VISITORS + void visit(AstNodeIf* nodep) override { + if (m_ignoreRemaining) return; + const VisitBase vb{this, nodep}; + iterateAndNextConstNull(nodep->condp()); + const uint32_t savedCount = m_stackSize; + + UINFO(8, "thensp:\n"); + reset(); + iterateAndNextConstNull(nodep->thensp()); + uint32_t ifCount = m_stackSize; + if (nodep->branchPred().unlikely()) ifCount = 0; + + UINFO(8, "elsesp:\n"); + reset(); + iterateAndNextConstNull(nodep->elsesp()); + uint32_t elseCount = m_stackSize; + if (nodep->branchPred().likely()) elseCount = 0; + + reset(); + if (ifCount >= elseCount) { + m_stackSize = savedCount + ifCount; + if (nodep->elsesp()) nodep->elsesp()->user2(0); // Don't dump it + } else { + m_stackSize = savedCount + elseCount; + if (nodep->thensp()) nodep->thensp()->user2(0); // Don't dump it + } + } + void visit(AstNodeCond* nodep) override { + if (m_ignoreRemaining) return; + // Just like if/else above, the ternary operator only evaluates + // one of the two expressions, so only count the max. + const VisitBase vb{this, nodep}; + iterateAndNextConstNull(nodep->condp()); + const uint32_t savedCount = m_stackSize; + + UINFO(8, "?\n"); + reset(); + iterateAndNextConstNull(nodep->thenp()); + const uint32_t ifCount = m_stackSize; + + UINFO(8, ":\n"); + reset(); + iterateAndNextConstNull(nodep->elsep()); + const uint32_t elseCount = m_stackSize; + + reset(); + if (ifCount >= elseCount) { + m_stackSize = savedCount + ifCount; + if (nodep->elsep()) nodep->elsep()->user2(0); // Don't dump it + } else { + m_stackSize = savedCount + elseCount; + if (nodep->thenp()) nodep->thenp()->user2(0); // Don't dump it + } + } + void visit(AstFork* nodep) override { + if (m_ignoreRemaining) return; + const VisitBase vb{this, nodep}; + uint32_t totalCount = m_stackSize; + VL_RESTORER(m_ignoreRemaining); + // Sum counts in each statement + for (AstNode* stmtp = nodep->stmtsp(); stmtp; stmtp = stmtp->nextp()) { + reset(); + iterateConst(stmtp); + totalCount += m_stackSize; + } + m_stackSize = totalCount; + } + void visit(AstNodeCCall* nodep) override { + if (m_ignoreRemaining) return; + const VisitBase vb{this, nodep}; + iterateChildrenConst(nodep); + m_tracingCall = true; + iterateConst(nodep->funcp()); + UASSERT_OBJ(!m_tracingCall, nodep, "visit(AstCFunc) should have cleared m_tracingCall."); + } + void visit(AstCFunc* nodep) override { + // Don't count a CFunc other than by tracing a call or counting it + // from the root + if (!m_tracingCall && !nodep->entryPoint()) return; + m_tracingCall = false; + if (nodep->recursive()) return; + if (!nodep->user2()) { // Short circuit + VL_RESTORER(m_ignoreRemaining); + VL_RESTORER(m_stackSize); + VL_RESTORER(m_inCFunc); + m_tracingCall = false; + m_stackSize = 0; + m_inCFunc = true; + const VisitBase vb{this, nodep}; + iterateChildrenConst(nodep); + nodep->user2(m_stackSize + 1); + } + m_stackSize += nodep->user2() - 1; + m_tracingCall = false; + } + void visit(AstVar* nodep) override { + if (!m_inCFunc) return; + m_stackSize += nodep->isRef() ? sizeof(void*) : nodep->dtypep()->widthTotalBytes(); + iterateChildrenConst(nodep); + } + + void visit(AstNodeExpr* nodep) override {} // Short-circuit + void visit(AstNode* nodep) override { + if (m_ignoreRemaining) return; + const VisitBase vb{this, nodep}; + iterateChildrenConst(nodep); + } + + VL_UNCOPYABLE(StackCountVisitor); +}; + +uint64_t V3StackCount::count(AstNode* nodep) { + const StackCountVisitor visitor{nodep}; + return visitor.stackSize(); +} diff --git a/src/V3StackCount.h b/src/V3StackCount.h new file mode 100644 index 000000000..3a8e15d50 --- /dev/null +++ b/src/V3StackCount.h @@ -0,0 +1,38 @@ +// -*- mode: C++; c-file-style: "cc-mode" -*- +//************************************************************************* +// DESCRIPTION: Verilator: Estimate stackuction count to run the logic +// we would generate for any given AST subtree. +// +// Code available from: https://verilator.org +// +//************************************************************************* +// +// Copyright 2003-2024 by Wilson Snyder. This program is free software; you +// can redistribute it and/or modify it under the terms of either the GNU +// Lesser General Public License Version 3 or the Perl Artistic License +// Version 2.0. +// SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 +// +//************************************************************************* + +#ifndef VERILATOR_V3STACKCOUNT_H_ +#define VERILATOR_V3STACKCOUNT_H_ + +#include "config_build.h" +#include "verilatedos.h" + +#include "V3ThreadSafety.h" + +class AstNode; + +class V3StackCount final { +public: + // Return the estimate count of bytes needed in stack we'd incur while + // running code in and under nodep. + // + // This is a rough estimate; we don't know what path we'll take through + // conditionals in nodep, so we assume we take the longest path. + static uint64_t count(AstNode* nodep) VL_MT_DISABLED; +}; + +#endif // guard diff --git a/test_regress/t/t_stack_check.pl b/test_regress/t/t_stack_check.pl new file mode 100755 index 000000000..8c196698f --- /dev/null +++ b/test_regress/t/t_stack_check.pl @@ -0,0 +1,24 @@ +#!/usr/bin/env perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2003-2024 by Wilson Snyder. This program is free software; you +# can redistribute it and/or modify it under the terms of either the GNU +# Lesser General Public License Version 3 or the Perl Artistic License +# Version 2.0. +# SPDX-License-Identifier: LGPL-3.0-only OR Artistic-2.0 + +scenarios(vlt => 1); + +compile( + verilator_flags2 => ['--binary --debug-stack-check'], + verilator_make_cmake => 0, + verilator_make_gmake => 0, + make_main => 0, + ); + +execute(); + +file_grep($Self->{run_log_filename}, qr/.*%Warning: System has stack size/); +ok(1); +1; diff --git a/test_regress/t/t_stack_check.v b/test_regress/t/t_stack_check.v new file mode 100644 index 000000000..e95f4535f --- /dev/null +++ b/test_regress/t/t_stack_check.v @@ -0,0 +1,14 @@ +// DESCRIPTION: Verilator: Verilog Test module +// +// This file ONLY is placed under the Creative Commons Public Domain, for +// any use, without warranty, 2024 by Wilson Snyder. +// SPDX-License-Identifier: CC0-1.0 + +module t (/*AUTOARG*/); + + initial begin + $write("*-* All Finished *-*\n"); + $finish; + end + +endmodule