Change metacomments to not enable warnings disabled in control file (#6836) (#6842)

Track the location based message/feature enable bits separately for code
and control file directives. A message/feature is disabled if disabled
either in the control file, or in code directives/metacomments. That is,
enabled only if both agree should be enabled.

docs/guide/exe_verilator.rst

@@ -2382,8 +2382,10 @@ The grammar of control commands is as follows:
    (or wildcard with '\*' or '?', or all files if omitted) and range of
    line numbers (or all lines if omitted).
 
-   With lint_off using "\*" will override any lint_on directives in the
+   If a warning is disabled with lint_off, it will not be printed, even if the
-   source, i.e. the warning will still not be printed.
+   source contains a lint_on metacomment. The control file directives and
+   metacomments are interpreted separately and do not interact. A warning is
+   emitted only if not disabled either in a control file or via metacomments.
 
    If the ``-rule`` is omitted, all lint warnings (see list in
    :vlopt:`-Wno-lint`) are enabled/disabled. This will override all later
@@ -2492,9 +2494,10 @@ The grammar of control commands is as follows:
    :option:`--no-timing`, and code:`fork`/``join*`` blocks are
    converted into ``begin``/``end`` blocks.
 
-   Same as :option:`/*verilator&32;timing_on*/`,
+   Similar to :option:`/*verilator&32;timing_on*/`,
-   :option:`/*verilator&32;timing_off*/` metacomments.
+   :option:`/*verilator&32;timing_off*/` metacomments, but interpreted
-
+   independtly. If either a control file, or metacommets disable timing
+   constructs, they will be disabled.
 
    .. t_dist_docs_style ignore tracing_on
 

docs/guide/warnings.rst

@@ -45,6 +45,9 @@ Warnings may be disabled in multiple ways:
 
       lint_off -rule UNSIGNED -file "*/example.v" -lines 1
 
+Metacomments and control file directives do not interact. If a warning is
+disabled by either metacomments, or a directive in a control file, it will not
+be emitted.
 
 Error And Warning Format
 ========================

src/V3Control.cpp

@@ -432,7 +432,7 @@ public:
             for (; m_lastIgnore.it != m_ignLines.end(); ++m_lastIgnore.it) {
                 if (m_lastIgnore.it->m_lineno > curlineno) break;
                 // UINFO(9, "     Hit " << *m_lastIgnore.it);
-                filelinep->warnOn(m_lastIgnore.it->m_code, m_lastIgnore.it->m_on);
+                filelinep->warnOnCtrl(m_lastIgnore.it->m_code, m_lastIgnore.it->m_on);
             }
             if (false && debug() >= 9) {
                 for (IgnLines::const_iterator it = m_lastIgnore.it; it != m_ignLines.end(); ++it) {

src/V3FileLine.cpp

@@ -126,26 +126,32 @@ FileLineSingleton::msgEnSetIdx_t FileLineSingleton::addMsgEnBitSet(const MsgEnBi
 FileLineSingleton::msgEnSetIdx_t FileLineSingleton::defaultMsgEnIndex() VL_MT_SAFE {
     MsgEnBitSet msgEnBitSet;
     for (int i = V3ErrorCode::EC_MIN; i < V3ErrorCode::_ENUM_MAX; ++i) {
-        msgEnBitSet.set(i, !V3ErrorCode{i}.defaultsOff());
+        // "-Wall" and the like only adjsut the code subset, so use default enablement there
+        msgEnBitSet.set(MsgEnBitSet::Subset::CODE, i, !V3ErrorCode{i}.defaultsOff());
+        // The control file subset is only adjusted by the control files, everything enabled by
+        // default
+        msgEnBitSet.set(MsgEnBitSet::Subset::CTRL, i, true);
     }
     return addMsgEnBitSet(msgEnBitSet);
 }
 
 FileLineSingleton::msgEnSetIdx_t FileLineSingleton::msgEnSetBit(msgEnSetIdx_t setIdx,
+                                                                MsgEnBitSet::Subset subset,
                                                                 size_t bitIdx, bool value) {
-    if (msgEn(setIdx).test(bitIdx) == value) return setIdx;
+    if (msgEn(setIdx).test(subset, bitIdx) == value) return setIdx;
     MsgEnBitSet msgEnBitSet{msgEn(setIdx)};
-    msgEnBitSet.set(bitIdx, value);
+    msgEnBitSet.set(subset, bitIdx, value);
     return addMsgEnBitSet(msgEnBitSet);
 }
 
 FileLineSingleton::msgEnSetIdx_t FileLineSingleton::msgEnAnd(msgEnSetIdx_t lhsIdx,
                                                              msgEnSetIdx_t rhsIdx) {
-    MsgEnBitSet msgEnBitSet{msgEn(lhsIdx)};
+    const MsgEnBitSet& lhs = msgEn(lhsIdx);
-    msgEnBitSet &= msgEn(rhsIdx);
+    const MsgEnBitSet& rhs = msgEn(rhsIdx);
-    if (msgEnBitSet == msgEn(lhsIdx)) return lhsIdx;
+    const MsgEnBitSet intersection{lhs, rhs};
-    if (msgEnBitSet == msgEn(rhsIdx)) return rhsIdx;
+    if (intersection == lhs) return lhsIdx;
-    return addMsgEnBitSet(msgEnBitSet);
+    if (intersection == rhs) return rhsIdx;
+    return addMsgEnBitSet(intersection);
 }
 
 // ######################################################################
@@ -415,12 +421,12 @@ void FileLine::warnUnusedOff(bool flag) {
 }
 
 bool FileLine::warnIsOff(V3ErrorCode code) const {
-    if (!msgEn().test(code)) return true;
+    if (!msgEn().enabled(code)) return true;
-    if (!defaultFileLine().msgEn().test(code)) return true;  // Global overrides local
+    if (!defaultFileLine().msgEn().enabled(code)) return true;  // Global overrides local
-    if ((code.lintError() || code.styleError()) && !msgEn().test(V3ErrorCode::I_LINT)) {
+    if ((code.lintError() || code.styleError()) && !msgEn().enabled(V3ErrorCode::I_LINT)) {
         return true;
     }
-    if ((code.unusedError()) && !msgEn().test(V3ErrorCode::I_UNUSED)) return true;
+    if ((code.unusedError()) && !msgEn().enabled(V3ErrorCode::I_UNUSED)) return true;
     return false;
 }
 

src/V3FileLine.h

@@ -21,6 +21,7 @@
 #include "verilatedos.h"
 
 #include "V3Error.h"
+#include "V3Hash.h"
 #include "V3LangCode.h"
 #include "V3Mutex.h"
 
@@ -48,7 +49,58 @@ class FileLineSingleton final {
     // TYPES
     using fileNameIdx_t = uint16_t;  // Increase width if 64K input files are not enough
     using msgEnSetIdx_t = uint16_t;  // Increase width if 64K unique message sets are not enough
-    using MsgEnBitSet = std::bitset<V3ErrorCode::_ENUM_MAX>;
+    class MsgEnBitSet final {
+        std::bitset<V3ErrorCode::_ENUM_MAX> m_codeEn;  // Enabeld by code directives/metacomments
+        std::bitset<V3ErrorCode::_ENUM_MAX> m_ctrlEn;  // Enabled by control file
+
+    public:
+        enum class Subset {
+            CODE = 0,  // Selects m_codeEn, the enable bits used by in-code directives/metacomments
+            CTRL = 1,  // Selects m_ctrlEn, the enable bits used by control files
+        };
+
+        // Create empty set
+        MsgEnBitSet() = default;
+        // Create intersection set
+        MsgEnBitSet(const MsgEnBitSet& a, const MsgEnBitSet& b)
+            : m_codeEn{a.m_codeEn & b.m_codeEn}
+            , m_ctrlEn{a.m_ctrlEn & b.m_ctrlEn} {}
+
+        struct Hash final {
+            size_t operator()(const MsgEnBitSet& item) const {
+                const size_t hashCode
+                    = std::hash<std::bitset<V3ErrorCode::_ENUM_MAX>>()(item.m_codeEn);
+                const size_t hashCtrl
+                    = std::hash<std::bitset<V3ErrorCode::_ENUM_MAX>>()(item.m_ctrlEn);
+                V3Hash hash{static_cast<uint64_t>(hashCode)};
+                hash += static_cast<uint64_t>(hashCtrl);
+                return hash.value();
+            }
+        };
+
+        struct Equal final {
+            bool operator()(const MsgEnBitSet& a, const MsgEnBitSet& b) const { return a == b; }
+        };
+
+        bool operator==(const MsgEnBitSet& other) const {
+            return m_codeEn == other.m_codeEn && m_ctrlEn == other.m_ctrlEn;
+        }
+
+        bool test(Subset subset, size_t code) const {
+            return subset == Subset::CODE ? m_codeEn.test(code) : m_ctrlEn.test(code);
+        }
+
+        void set(Subset subset, size_t code, bool value) {
+            if (subset == Subset::CODE) {
+                m_codeEn.set(code, value);
+            } else {
+                m_ctrlEn.set(code, value);
+            }
+        }
+
+        // Enabled iff enabled by both in-code dierctives/metacomments and control file
+        bool enabled(V3ErrorCode code) const { return m_codeEn.test(code) && m_ctrlEn.test(code); }
+    };
 
     // MEMBERS
     V3Mutex m_mutex;  // protects members
@@ -57,7 +109,8 @@ class FileLineSingleton final {
     std::deque<V3LangCode> m_languages;  // language for each filenameno
 
     // Map from flag set to the index in m_internedMsgEns for interning
-    std::unordered_map<MsgEnBitSet, msgEnSetIdx_t> m_internedMsgEnIdxs VL_GUARDED_BY(m_mutex);
+    std::unordered_map<MsgEnBitSet, msgEnSetIdx_t, MsgEnBitSet::Hash, MsgEnBitSet::Equal>
+        m_internedMsgEnIdxs VL_GUARDED_BY(m_mutex);
     // Interned message enablement flag sets
     std::vector<MsgEnBitSet> m_internedMsgEns;
 
@@ -87,7 +140,8 @@ class FileLineSingleton final {
     // Add index of default bitset
     msgEnSetIdx_t defaultMsgEnIndex() VL_MT_SAFE;
     // Set bitIdx to value in bitset at interned index setIdx, return interned index of result
-    msgEnSetIdx_t msgEnSetBit(msgEnSetIdx_t setIdx, size_t bitIdx, bool value);
+    msgEnSetIdx_t msgEnSetBit(msgEnSetIdx_t setIdx, MsgEnBitSet::Subset subset, size_t bitIdx,
+                              bool value);
     // Return index to intersection set
     msgEnSetIdx_t msgEnAnd(msgEnSetIdx_t lhsIdx, msgEnSetIdx_t rhsIdx);
     // Retrieve interned bitset at given interned index. The returned reference is not persistent.
@@ -312,14 +366,21 @@ public:
     string lineDirectiveStrg(int enterExit) const;
 
     // Turn on/off warning messages on this line.
-    void warnOn(V3ErrorCode code, bool flag) {
+private:
+    void warnSet(MsgEnBitSet::Subset subset, V3ErrorCode code, bool flag) {
         if (code == V3ErrorCode::WIDTH) {
-            warnOn(V3ErrorCode::WIDTHTRUNC, flag);
+            warnSet(subset, V3ErrorCode::WIDTHTRUNC, flag);
-            warnOn(V3ErrorCode::WIDTHEXPAND, flag);
+            warnSet(subset, V3ErrorCode::WIDTHEXPAND, flag);
-            warnOn(V3ErrorCode::WIDTHXZEXPAND, flag);
+            warnSet(subset, V3ErrorCode::WIDTHXZEXPAND, flag);
         }
-        if (code == V3ErrorCode::E_UNSUPPORTED) warnOn(V3ErrorCode::COVERIGN, flag);
+        if (code == V3ErrorCode::E_UNSUPPORTED) { warnSet(subset, V3ErrorCode::COVERIGN, flag); }
-        m_msgEnIdx = singleton().msgEnSetBit(m_msgEnIdx, code, flag);
+        m_msgEnIdx = singleton().msgEnSetBit(m_msgEnIdx, subset, code, flag);
+    }
+
+public:
+    void warnOn(V3ErrorCode code, bool flag) { warnSet(MsgEnBitSet::Subset::CODE, code, flag); }
+    void warnOnCtrl(V3ErrorCode code, bool flag) {
+        warnSet(MsgEnBitSet::Subset::CTRL, code, flag);
     }
     void warnOff(V3ErrorCode code, bool flag) { warnOn(code, !flag); }
     string warnOffParse(const string& msgs, bool flag);  // Returns "" if ok
@@ -331,13 +392,13 @@ public:
     void warnResetDefault() { warnStateFrom(defaultFileLine()); }
 
     // Specific flag ACCESSORS/METHODS
-    bool celldefineOn() const { return msgEn().test(V3ErrorCode::I_CELLDEFINE); }
+    bool celldefineOn() const { return msgEn().enabled(V3ErrorCode::I_CELLDEFINE); }
     void celldefineOn(bool flag) { warnOn(V3ErrorCode::I_CELLDEFINE, flag); }
-    bool coverageOn() const { return msgEn().test(V3ErrorCode::I_COVERAGE); }
+    bool coverageOn() const { return msgEn().enabled(V3ErrorCode::I_COVERAGE); }
     void coverageOn(bool flag) { warnOn(V3ErrorCode::I_COVERAGE, flag); }
-    bool tracingOn() const { return msgEn().test(V3ErrorCode::I_TRACING); }
+    bool tracingOn() const { return msgEn().enabled(V3ErrorCode::I_TRACING); }
     void tracingOn(bool flag) { warnOn(V3ErrorCode::I_TRACING, flag); }
-    bool timingOn() const { return msgEn().test(V3ErrorCode::I_TIMING); }
+    bool timingOn() const { return msgEn().enabled(V3ErrorCode::I_TIMING); }
     void timingOn(bool flag) { warnOn(V3ErrorCode::I_TIMING, flag); }
 
     // METHODS - Global
@@ -413,8 +474,12 @@ public:
         if (m_lastLinenoAdder != rhs.m_lastLinenoAdder)
             return (m_lastLinenoAdder < rhs.m_lastLinenoAdder) ? -1 : 1;
         if (m_lastColumn != rhs.m_lastColumn) return (m_lastColumn < rhs.m_lastColumn) ? -1 : 1;
-        for (size_t i = 0; i < msgEn().size(); ++i) {
+        const MsgEnBitSet& lhsMsgEn = msgEn();
-            if (msgEn().test(i) != rhs.msgEn().test(i)) return rhs.msgEn().test(i) ? -1 : 1;
+        const MsgEnBitSet& rhsMsgEn = rhs.msgEn();
+        for (size_t i = 0; i < V3ErrorCode::_ENUM_MAX; ++i) {
+            V3ErrorCode code = static_cast<V3ErrorCode>(i);
+            if (lhsMsgEn.enabled(code) != rhsMsgEn.enabled(code))
+                return rhsMsgEn.enabled(code) ? -1 : 1;
         }
         // TokenNum is compared last as makes more logical sort order by file/line first
         if (m_tokenNum != rhs.m_tokenNum) return (m_tokenNum < rhs.m_tokenNum) ? -1 : 1;

test_regress/t/t_vlt_timing.py

@@ -10,7 +10,6 @@
 import vltest_bootstrap
 
 test.scenarios('simulator')
-test.top_filename = "t/t_timing_off.v"
 
 test.compile(verilator_flags2=["--binary t/t_vlt_timing.vlt"])
 

test_regress/t/t_vlt_timing.v

@@ -0,0 +1,39 @@
+// DESCRIPTION: Verilator: Verilog Test module
+//
+// This file ONLY is placed under the Creative Commons Public Domain, for
+// any use, without warranty, 2022 by Antmicro Ltd.
+// SPDX-License-Identifier: CC0-1.0
+
+module t;
+  event e1;
+  event e2;
+
+  initial begin
+    int x;
+    // verilator timing_off
+    #1
+    fork @e1; @e2; join;
+    @e1
+    wait(x == 4)
+    x = #1 8;
+    // verilator timing_on
+    if (x != 8) $stop;
+    if ($time != 0) $stop;
+
+    @e2;
+
+    @e1;
+    if ((e1.triggered && e2.triggered)
+      || (!e1.triggered && !e2.triggered)) $stop;
+    if ($time != 2) $stop;
+    $write("*-* All Finished *-*\n");
+    $finish;
+  end
+
+  initial #2 ->e1;
+
+  initial #2 ->e2;
+
+  initial #3 $stop; // timeout
+  initial #1 @(e1, e2) #1 $stop; // timeout
+endmodule

test_regress/t/t_vlt_timing.vlt

@@ -6,6 +6,10 @@
 
 `verilator_config
 
-timing_on --file "t/t_timing_off.v" --lines 23
+timing_off --file "t/t_vlt_timing.v"
-timing_off -file "t/t_timing_off.v" -lines 26-34
+timing_on -file "t/t_vlt_timing.v" --lines 23
-timing_on -file "t/t_timing_off.v" -lines 35-38
+// Bug here. This line should make no difference.
+//timing_off --file "t/t_vlt_timing.v" --lines 22-24
+timing_on --file "t/t_vlt_timing.v" --lines 23
+timing_off -file "t/t_vlt_timing.v" -lines 26-34
+timing_on -file "t/t_vlt_timing.v" -lines 35-38

test_regress/t/t_vlt_warn.v

@@ -20,6 +20,10 @@ module t;
    reg width_warn2_var_line19 = 2'b11;  // Width warning - must be line 19
    reg width_warn3_var_line20 = 2'b11;  // Width warning - must be line 20
 
+   // Must not turn back on warning disabled via control file
+   // verilator lint_on CASEINCOMPLETE
+   // verilator lint_on CASEX
+
    initial begin
       casex (1'b1)
         1'b0: $stop;