Add checking for config instance rules that target hierarchy undernea…

…th another config block
MikePopoloski · Mar 4, 2024 · bc3bf24 · bc3bf24
1 parent 5e5fdfd
commit bc3bf24
Show file tree

Hide file tree

Showing 5 changed files with 65 additions and 4 deletions.
diff --git a/include/slang/ast/symbols/CompilationUnitSymbols.h b/include/slang/ast/symbols/CompilationUnitSymbols.h
@@ -272,8 +272,10 @@ class SLANG_EXPORT ConfigBlockSymbol : public Symbol, public Scope {
     struct TopCell {
         const DefinitionSymbol& definition;
         ConfigRule* rule = nullptr;
+        SourceRange sourceRange;
 
-        explicit TopCell(const DefinitionSymbol& definition) : definition(definition) {}
+        TopCell(const DefinitionSymbol& definition, SourceRange sourceRange) :
+            definition(definition), sourceRange(sourceRange) {}
     };
 
     struct CellOverride {

diff --git a/scripts/diagnostics.txt b/scripts/diagnostics.txt
@@ -500,6 +500,7 @@ error ExternIfaceArrayMethod "cannot connect extern subroutine to interface arra
 error ConfigInstanceWrongTop "config instance path must start with one of the cells named in the 'design' statement"
 error ConfigDupTop "config design specifies more than one top cell named '{}'"
 error ConfigOverrideTop "config rule can't override a top cell with a different target"
+error ConfigInstanceUnderOtherConfig "config instance rule applies to an instance that is within a hierarchy specified by another config"
 error FatalTask "$fatal encountered{}"
 error ErrorTask "$error encountered{}"
 error StaticAssert "static assertion failed{}"

diff --git a/source/ast/symbols/CompilationUnitSymbols.cpp b/source/ast/symbols/CompilationUnitSymbols.cpp
@@ -423,7 +423,7 @@ void ConfigBlockSymbol::resolve() const {
 
             auto [it, inserted] = topCellNames.emplace(cellName, topCellsBuf.size());
             if (inserted)
-                topCellsBuf.emplace_back(*def);
+                topCellsBuf.emplace_back(*def, cellId->sourceRange());
             else
                 scope->addDiag(diag::ConfigDupTop, cellId->cell.range()) << cellName;
         }

diff --git a/source/ast/symbols/InstanceSymbols.cpp b/source/ast/symbols/InstanceSymbols.cpp
@@ -394,6 +394,22 @@ static const ConfigBlockSymbol::InstanceOverride* findInstanceOverrideNode(
     return node->childNodes.empty() ? nullptr : node;
 }
 
+static void checkForInvalidNestedConfigNodes(const ASTContext& context,
+                                             const ConfigBlockSymbol::InstanceOverride& node,
+                                             const ConfigBlockSymbol& configBlock) {
+    if (node.rule) {
+        // Mark the rule as used so we don't also get a warning
+        // about this rule when we're going to give a hard error anyway.
+        node.rule->isUsed = true;
+
+        auto& diag = context.addDiag(diag::ConfigInstanceUnderOtherConfig, node.rule->sourceRange);
+        diag.addNote(diag::NoteConfigRule, configBlock.getTopCells()[0].sourceRange);
+    }
+
+    for (auto& [_, child] : node.childNodes)
+        checkForInvalidNestedConfigNodes(context, child, configBlock);
+}
+
 void InstanceSymbol::fromSyntax(Compilation& comp, const HierarchyInstantiationSyntax& syntax,
                                 const ASTContext& context, SmallVectorBase<const Symbol*>& results,
                                 SmallVectorBase<const Symbol*>& implicitNets, bool isFromBind) {
@@ -581,16 +597,19 @@ void InstanceSymbol::fromSyntax(Compilation& comp, const HierarchyInstantiationS
                     std::optional<Compilation::DefinitionLookupResult> explicitDef;
                     auto& overrideMap = overrideNode->childNodes;
                     for (auto instSyntax : syntax.instances) {
+                        const ConfigBlockSymbol* nestedConfig = nullptr;
                         auto instName = instSyntax->decl ? instSyntax->decl->name.valueText()
                                                          : ""sv;
-                        if (auto ruleIt = overrideMap.find(instName);
-                            ruleIt != overrideMap.end() && ruleIt->second.rule) {
+
+                        auto ruleIt = overrideMap.find(instName);
+                        if (ruleIt != overrideMap.end() && ruleIt->second.rule) {
                             // We have an override rule, so use it to lookup the def.
                             auto defResult = comp.getDefinition(defName, *context.scope,
                                                                 *ruleIt->second.rule,
                                                                 instSyntax->sourceRange(),
                                                                 diag::UnknownModule);
                             createInstances(defResult, instSyntax);
+                            nestedConfig = defResult.configRoot;
                         }
                         else {
                             // No specific config rule, so use the default lookup behavior.
@@ -600,6 +619,15 @@ void InstanceSymbol::fromSyntax(Compilation& comp, const HierarchyInstantiationS
                                                                  diag::UnknownModule);
                             }
                             createInstances(*explicitDef, instSyntax);
+                            nestedConfig = explicitDef->configRoot;
+                        }
+
+                        // If we found a new nested config block that applies hierarchically
+                        // to our instance and children, any other rules that would apply
+                        // further down the tree from the original config are invalid.
+                        if (nestedConfig && ruleIt != overrideMap.end()) {
+                            for (auto& [_, child] : ruleIt->second.childNodes)
+                                checkForInvalidNestedConfigNodes(context, child, *nestedConfig);
                         }
                     }
                     return;

diff --git a/tests/unittests/ast/ConfigTests.cpp b/tests/unittests/ast/ConfigTests.cpp
@@ -851,3 +851,33 @@ endconfig
     REQUIRE(diags.size() == 1);
     CHECK(diags[0].code == diag::Redefinition);
 }
+
+TEST_CASE("Config instance path underneath a hierarchical config error") {
+    auto tree = SyntaxTree::fromText(R"(
+config cfg1;
+    design top;
+    cell f use cfg2 : config;
+    instance top.foo.q use l;
+endconfig
+
+config cfg2;
+    design bar;
+endconfig
+
+module bar;
+endmodule
+
+module top;
+    f foo();
+endmodule
+)");
+    CompilationOptions options;
+    options.topModules.emplace("cfg1");
+
+    Compilation compilation(options);
+    compilation.addSyntaxTree(tree);
+
+    auto& diags = compilation.getAllDiagnostics();
+    REQUIRE(diags.size() == 1);
+    CHECK(diags[0].code == diag::ConfigInstanceUnderOtherConfig);
+}