From 53a547e0bf635bbffe1483e8b15c42df45831cc9 Mon Sep 17 00:00:00 2001 From: Yan Churkin Date: Tue, 20 Aug 2024 14:24:21 +0300 Subject: [PATCH] [slang] Fix UDP overlap bugs --- source/ast/symbols/MemberSymbols.cpp | 125 ++++++++++++------------- tests/unittests/ast/PrimitiveTests.cpp | 33 ++++++- 2 files changed, 93 insertions(+), 65 deletions(-) diff --git a/source/ast/symbols/MemberSymbols.cpp b/source/ast/symbols/MemberSymbols.cpp index b9ab970f0..6427b5c46 100644 --- a/source/ast/symbols/MemberSymbols.cpp +++ b/source/ast/symbols/MemberSymbols.cpp @@ -9,6 +9,7 @@ #include "../FmtHelpers.h" #include "fmt/core.h" +#include #include "slang/ast/ASTSerializer.h" #include "slang/ast/ASTVisitor.h" @@ -766,13 +767,14 @@ void PrimitivePortSymbol::serializeTo(ASTSerializer& serializer) const { // Each 'bit' is one input value in the row. class BitTrie { public: - // Tries to insert the row. Returns nullptr if the row is - // successfully inserted, and a pointer to the previously - // inserted row with the same bit pattern if a collision + // Tries to insert the row. Returns nullopt if the row is + // successfully inserted, and a set of the previously + // inserted rows with the same bit pattern if a collision // is found. template - const UdpEntrySyntax* insert(const UdpEntrySyntax& syntax, std::span inputs, - char stateChar, TAllocator& allocator) { + std::optional> insert(const UdpEntrySyntax& syntax, + std::span inputs, + char stateChar, TAllocator& allocator) { SmallVector nodes; BitTrie* primary = this; nodes.push_back(this); @@ -808,14 +810,13 @@ class BitTrie { advance('0'); break; case 'p': - case 'n': { - // Special handling for iteration of edge specifiers. - SmallVector nextNodes; - for (auto node : nodes) - node->nextNodesForEdge(c, nextNodes, primary, allocator); - nodes = std::move(nextNodes); + advance('6'); + advance('7'); + break; + case 'n': + advance('7'); + advance('6'); break; - } default: break; } @@ -827,17 +828,24 @@ class BitTrie { // If any of our nodes have entries we won't insert, // as it means we have a duplicate. + std::set eSyntaxes; for (auto node : nodes) { if (node->entry) - return node->entry; + eSyntaxes.insert(node->entry); } - // If we have an empty node set we saw an error somewhere, + // Always store so as not to miss info it in case of possible overlap. + // If the primary->entry already has a value, + // then rewriting will not spoil anything, + // since the rewriting will be to the equivalent grammar. + primary->entry = &syntax; + + // If we have an empty syntaxes set we saw an error somewhere, // in which case don't insert. Otherwise we've found the // correct place to insert, pointed to by the primary. - if (!nodes.empty()) - primary->entry = &syntax; - return nullptr; + if (eSyntaxes.empty()) + return std::nullopt; + return eSyntaxes; } private: @@ -868,15 +876,19 @@ class BitTrie { handle(0, true); handle(3); handle(4); + handle(6); break; case '1': handle(1, true); handle(3); handle(4); + handle(7); break; case 'x': handle(2, true); handle(3); + handle(6); + handle(7); break; case '?': handle(3, true); @@ -884,6 +896,8 @@ class BitTrie { handle(1); handle(2); handle(4); + handle(6); + handle(7); break; case 'b': handle(4, true); @@ -899,6 +913,21 @@ class BitTrie { case 'n': handle(5, true); break; + // Below are implicit node identifiers that cannot be found in the UDP grammar. They are + // helpers for `p` and `n`. Handling `0` or `x` (for `p` and `r` matching cases) + case '6': + handle(6, true); + handle(0); + handle(2); + handle(3); + break; + // Handling `1` or `x` (for `p` and `r` matching cases) + case '7': + handle(7, true); + handle(1); + handle(2); + handle(3); + break; default: // On error clear all nodes. Assume someone else // (the parser) has reported the error already. @@ -907,41 +936,7 @@ class BitTrie { } } - template - void nextNodesForEdge(char c, SmallVector& nextNodes, BitTrie*& primaryNode, - TAllocator& allocator) { - SmallVector results; - auto handle = [&](char first, char second) { - // We have to do this in two steps when expanding the edge char. - BitTrie* unused = nullptr; - SmallVector temp; - nextNodesFor(first, temp, primaryNode, allocator); - for (auto node : temp) { - for (char d : {second, 'x'}) - node->nextNodesFor(d, results, d == second ? primaryNode : unused, allocator); - } - - temp.clear(); - nextNodesFor('x', temp, unused, allocator); - for (auto node : temp) - node->nextNodesFor(second, results, unused, allocator); - }; - - if (c == 'p') - handle('0', '1'); - else - handle('1', '0'); - - // We have to de-dup now because we iterated multiple times - // for the same slot. - SmallSet set; - for (auto node : results) { - if (set.emplace(node).second) - nextNodes.push_back(node); - } - } - - BitTrie* children[6] = {}; + BitTrie* children[8] = {}; const UdpEntrySyntax* entry = nullptr; }; @@ -1076,18 +1071,20 @@ static void createTableRow(const Scope& scope, const UdpEntrySyntax& syntax, } }; - auto existing = trie.insert(syntax, inputs, stateChar, trieAlloc); - if (existing) { - // This is an error if the existing row has a different output, - // otherwise it's just silently ignored. - auto existingOutput = getOutputChar(existing->next); - auto existingState = getStateChar(existing->current); - if (!(existingOutput == outputChar || - matchOutput(existingState, existingOutput, outputChar) || - matchOutput(stateChar, outputChar, existingOutput))) { - auto& diag = scope.addDiag(diag::UdpDupDiffOutput, syntax.sourceRange()); - diag.addNote(diag::NotePreviousDefinition, existing->sourceRange()); - return; + auto existingSyntaxes = trie.insert(syntax, inputs, stateChar, trieAlloc); + if (existingSyntaxes.has_value()) { + for (const auto* existing : existingSyntaxes.value()) { + // This is an error if the existing row has a different output, + // otherwise it's just silently ignored. + auto existingOutput = getOutputChar(existing->next); + auto existingState = getStateChar(existing->current); + if (!(existingOutput == outputChar || + matchOutput(existingState, existingOutput, outputChar) || + matchOutput(stateChar, outputChar, existingOutput))) { + auto& diag = scope.addDiag(diag::UdpDupDiffOutput, syntax.sourceRange()); + diag.addNote(diag::NotePreviousDefinition, existing->sourceRange()); + return; + } } } else if (allX && outputChar != 'x') { diff --git a/tests/unittests/ast/PrimitiveTests.cpp b/tests/unittests/ast/PrimitiveTests.cpp index f2b8a1cae..68ca06006 100644 --- a/tests/unittests/ast/PrimitiveTests.cpp +++ b/tests/unittests/ast/PrimitiveTests.cpp @@ -433,8 +433,36 @@ primitive p(output reg o, input a, b); endtable endprimitive +primitive pp (q, clock, data); + output q; reg q; + input clock, data; + table + // clock data q q+ + p ? : ? : 1 ; + (x1) ? : ? : 0; + n ? : ? : 1 ; + (1x) ? : ? : 0; + ? (??) : ? : - ; + endtable +endprimitive + +primitive ppp (q, clock, data); + output q; reg q; + input clock, data; + table + // clock data q q+ + (?0) ? : ? : 1 ; + (0?) ? : ? : 1 ; + (01) ? : ? : 0 ; + (??) ? : ? : - ; + ? (??) : ? : - ; + endtable +endprimitive + module top; p p1(o, a, b); + pp pp1(o, a, b); + ppp ppp1(o, a, b); endmodule )"); @@ -442,10 +470,13 @@ endmodule compilation.addSyntaxTree(tree); auto& diags = compilation.getAllDiagnostics(); - REQUIRE(diags.size() == 3); + REQUIRE(diags.size() == 6); CHECK(diags[0].code == diag::UdpDupDiffOutput); CHECK(diags[1].code == diag::UdpDupDiffOutput); CHECK(diags[2].code == diag::UdpDupDiffOutput); + CHECK(diags[3].code == diag::UdpDupDiffOutput); + CHECK(diags[4].code == diag::UdpDupDiffOutput); + CHECK(diags[5].code == diag::UdpDupDiffOutput); } TEST_CASE("UDP overlapping inputs with compatible outputs") {