More error checking for DPI exports

MikePopoloski · Jan 10, 2021 · c31daa2 · c31daa2
1 parent 6211da6
commit c31daa2
Show file tree

Hide file tree

Showing 5 changed files with 193 additions and 17 deletions.
diff --git a/include/slang/compilation/Compilation.h b/include/slang/compilation/Compilation.h
@@ -30,6 +30,7 @@ class InstanceSymbol;
 class PackageSymbol;
 class RootSymbol;
 class Statement;
+class SubroutineSymbol;
 class SyntaxTree;
 class SystemSubroutine;
 
@@ -354,7 +355,7 @@ class Compilation : public BumpAllocator {
     Diagnostic& addDiag(Diagnostic diag);
 
     void parseParamOverrides(flat_hash_map<string_view, const ConstantValue*>& results);
-    void checkDPIExports();
+    void checkDPIMethods(span<const SubroutineSymbol* const> dpiImports);
 
     // Stored options object.
     CompilationOptions options;

diff --git a/scripts/diagnostics.txt b/scripts/diagnostics.txt
@@ -612,6 +612,10 @@ error InvalidParamOverrideOpt "'{}' is not a valid form of parameter override"
 error DPIExportKindMismatch "mismatch between 'function' and 'task' declared in DPI export"
 error DPIExportDifferentScope "exported subroutines must be defined in the same scope as the export directive"
 error DPIExportDuplicate "duplicate export of '{}'"
+error DPIExportImportedFunc "cannot export an imported DPI subroutine"
+error InvalidDPICIdentifier "'{}' is not a valid C identifier for DPI subroutine"
+error DPISignatureMismatch "more than one DPI subroutine with C identifier '{}' declared with mismatching type signatures"
+error DPIExportDuplicateCId "more than one DPI export with the same C identifier '{}' in the same scope"
 warning unused-def UnusedDefinition "{} definition is unused"
 warning no-top NoTopModules "no top-level modules found in design"
 

diff --git a/source/compilation/Compilation.cpp b/source/compilation/Compilation.cpp
@@ -6,6 +6,8 @@
 //------------------------------------------------------------------------------
 #include "slang/compilation/Compilation.h"
 
+#include "../text/CharInfo.h"
+
 #include "slang/binding/SystemSubroutine.h"
 #include "slang/compilation/Definition.h"
 #include "slang/compilation/ScriptSession.h"
@@ -163,6 +165,14 @@ struct DiagnosticVisitor : public ASTVisitor<DiagnosticVisitor, false, false> {
         handleDefault(symbol);
     }
 
+    void handle(const SubroutineSymbol& symbol) {
+        if (!handleDefault(symbol))
+            return;
+
+        if (symbol.flags.has(MethodFlags::DPIImport))
+            dpiImports.append(&symbol);
+    }
+
     void finalize() {
         // Once everything has been visited, go back over and check things that might
         // have been influenced by visiting later symbols. Unfortunately visiting
@@ -204,6 +214,7 @@ struct DiagnosticVisitor : public ASTVisitor<DiagnosticVisitor, false, false> {
     uint32_t errorLimit;
     uint32_t hierarchyDepth = 0;
     SmallVectorSized<const GenericClassDefSymbol*, 8> genericClasses;
+    SmallVectorSized<const SubroutineSymbol*, 4> dpiImports;
 };
 
 // This visitor is for finding all bind directives in the hierarchy.
@@ -863,9 +874,9 @@ const Diagnostics& Compilation::getSemanticDiagnostics() {
     getRoot().visit(visitor);
     visitor.finalize();
 
-    // Check all DPI exports for correctness.
-    if (!dpiExports.empty())
-        checkDPIExports();
+    // Check all DPI methods for correctness.
+    if (!dpiExports.empty() || !visitor.dpiImports.empty())
+        checkDPIMethods(visitor.dpiImports);
 
     // Report on unused out-of-block definitions. These are always a real error.
     for (auto& [key, val] : outOfBlockDecls) {
@@ -1127,7 +1138,73 @@ void Compilation::parseParamOverrides(flat_hash_map<string_view, const ConstantV
     }
 }
 
-void Compilation::checkDPIExports() {
+static bool isValidCIdChar(char c) {
+    return isAlphaNumeric(c) || c == '_';
+}
+
+static bool checkSignaturesMatch(const SubroutineSymbol& a, const SubroutineSymbol& b) {
+    if (a.subroutineKind != b.subroutineKind || a.flags != b.flags)
+        return false;
+
+    if (!a.getReturnType().isEquivalent(b.getReturnType()))
+        return false;
+
+    auto aargs = a.getArguments();
+    auto bargs = b.getArguments();
+    if (aargs.size() != bargs.size())
+        return false;
+
+    for (auto ai = aargs.begin(), bi = bargs.begin(); ai != aargs.end(); ai++, bi++) {
+        auto aa = *ai;
+        auto bb = *bi;
+        if (!aa->getType().isEquivalent(bb->getType()))
+            return false;
+        if (aa->direction != bb->direction)
+            return false;
+    }
+
+    return true;
+}
+
+void Compilation::checkDPIMethods(span<const SubroutineSymbol* const> dpiImports) {
+    auto getCId = [&](const Scope& scope, Token cid, Token name) {
+        string_view text = cid ? cid.valueText() : name.valueText();
+        if (!text.empty()) {
+            auto tail = text.substr(1);
+            if (!isValidCIdChar(text[0]) || isDecimalDigit(text[0]) ||
+                std::any_of(tail.begin(), tail.end(), [](char c) { return !isValidCIdChar(c); })) {
+                scope.addDiag(diag::InvalidDPICIdentifier, cid ? cid.range() : name.range())
+                    << text;
+                return string_view();
+            }
+        }
+        return text;
+    };
+
+    flat_hash_map<string_view, const SubroutineSymbol*> nameMap;
+    for (auto sub : dpiImports) {
+        auto syntax = sub->getSyntax();
+        ASSERT(syntax);
+
+        auto scope = sub->getParentScope();
+        ASSERT(scope);
+
+        auto& dis = syntax->as<DPIImportSyntax>();
+        string_view cId = getCId(*scope, dis.c_identifier, dis.method->name->getLastToken());
+        if (cId.empty())
+            continue;
+
+        auto [it, inserted] = nameMap.emplace(cId, sub);
+        if (!inserted) {
+            if (!checkSignaturesMatch(*sub, *it->second)) {
+                auto& diag = scope->addDiag(diag::DPISignatureMismatch, sub->location);
+                diag << cId;
+                diag.addNote(diag::NotePreviousDefinition, it->second->location);
+            }
+        }
+    }
+
+    flat_hash_map<std::tuple<string_view, const Scope*>, const DPIExportSyntax*> exportsByScope;
     flat_hash_map<const SubroutineSymbol*, const DPIExportSyntax*> previousExports;
     for (auto [syntax, scope] : dpiExports) {
         if (syntax->specString.valueText() == "DPI")
@@ -1164,6 +1241,12 @@ void Compilation::checkDPIExports() {
             continue;
         }
 
+        if (sub.flags.has(MethodFlags::DPIImport)) {
+            auto& diag = scope->addDiag(diag::DPIExportImportedFunc, syntax->name.range());
+            diag.addNote(diag::NoteDeclarationHere, sub.location);
+            continue;
+        }
+
         auto& retType = sub.getReturnType();
         if (!retType.isValidForDPIReturn() && !retType.isError()) {
             auto& diag = scope->addDiag(diag::InvalidDPIReturnType, sub.location);
@@ -1173,7 +1256,9 @@ void Compilation::checkDPIExports() {
         }
 
         for (auto arg : sub.getArguments()) {
-            // TODO: check input / output
+            if (arg->direction == ArgumentDirection::Ref)
+                scope->addDiag(diag::DPIRefArg, arg->location);
+
             auto& type = arg->getType();
             if (!type.isValidForDPIArg() && !type.isError()) {
                 auto& diag = scope->addDiag(diag::InvalidDPIArgType, arg->location);
@@ -1183,16 +1268,38 @@ void Compilation::checkDPIExports() {
             }
         }
 
-        auto [it, inserted] = previousExports.emplace(&sub, syntax);
-        if (!inserted) {
-            auto& diag = scope->addDiag(diag::DPIExportDuplicate, syntax->name.range()) << sub.name;
-            diag.addNote(diag::NotePreviousDefinition, it->second->name.location());
-            continue;
+        {
+            auto [it, inserted] = previousExports.emplace(&sub, syntax);
+            if (!inserted) {
+                auto& diag = scope->addDiag(diag::DPIExportDuplicate, syntax->name.range())
+                             << sub.name;
+                diag.addNote(diag::NotePreviousDefinition, it->second->name.location());
+                continue;
+            }
         }
 
-        // TODO:
-        // - check c_identifier
-        // - check no duplicate c_identifiers
+        string_view cId = getCId(*scope, syntax->c_identifier, syntax->name);
+        if (!cId.empty()) {
+            {
+                auto [it, inserted] = nameMap.emplace(cId, &sub);
+                if (!inserted) {
+                    if (!checkSignaturesMatch(sub, *it->second)) {
+                        auto& diag =
+                            scope->addDiag(diag::DPISignatureMismatch, syntax->name.range());
+                        diag << cId;
+                        diag.addNote(diag::NotePreviousDefinition, it->second->location);
+                    }
+                }
+            }
+            {
+                auto [it, inserted] = exportsByScope.emplace(std::make_tuple(cId, scope), syntax);
+                if (!inserted) {
+                    auto& diag = scope->addDiag(diag::DPIExportDuplicateCId, syntax->name.range());
+                    diag << cId;
+                    diag.addNote(diag::NotePreviousDefinition, it->second->name.location());
+                }
+            }
+        }
     }
 }
 

diff --git a/tests/unittests/ClassTests.cpp b/tests/unittests/ClassTests.cpp
@@ -2051,11 +2051,11 @@ endclass
 TEST_CASE("Randomize 'with' clauses") {
     auto tree = SyntaxTree::fromText(R"(
 package p;
-	int i;
+    int i;
     class Base;
         typedef int IntT;
     endclass
-	class A extends Base;
+    class A extends Base;
         rand int j;
         int l[5];
         function int foo; return 1; endfunction

diff --git a/tests/unittests/MemberTests.cpp b/tests/unittests/MemberTests.cpp
@@ -1183,14 +1183,25 @@ module m;
     export "DPI-C" function f1;
     export "DPI-C" function f2;
     export "DPI-C" function foo;
+
+    function void f3(ref r); endfunction
+    export "DPI-C" function f3;
+
+    import "DPI-C" function void f4;
+    export "DPI-C" function f4;
+
+    function void f5; endfunction
+    export "DPI-C" \0a = function f5;
+
+    import "DPI-C" a$ = function void f6;
 endmodule
 )");
 
     Compilation compilation;
     compilation.addSyntaxTree(tree);
 
     auto& diags = compilation.getAllDiagnostics();
-    REQUIRE(diags.size() == 8);
+    REQUIRE(diags.size() == 12);
     CHECK(diags[0].code == diag::DPISpecDisallowed);
     CHECK(diags[1].code == diag::TypoIdentifier);
     CHECK(diags[2].code == diag::NotASubroutine);
@@ -1199,6 +1210,59 @@ endmodule
     CHECK(diags[5].code == diag::InvalidDPIReturnType);
     CHECK(diags[6].code == diag::InvalidDPIArgType);
     CHECK(diags[7].code == diag::DPIExportDuplicate);
+    CHECK(diags[8].code == diag::DPIRefArg);
+    CHECK(diags[9].code == diag::DPIExportImportedFunc);
+    CHECK(diags[10].code == diag::InvalidDPICIdentifier);
+    CHECK(diags[11].code == diag::InvalidDPICIdentifier);
+}
+
+TEST_CASE("DPI signature checking") {
+    auto tree = SyntaxTree::fromText(R"(
+import "DPI-C" function int foo(int a, output b);
+
+function longint bar; endfunction
+export "DPI-C" foo = function bar;
+
+import "DPI-C" foo = function longint f1;
+
+function int f2(int a, output b); endfunction
+export "DPI-C" asdf = function f2;
+
+module m1;
+    function int f3(longint a, output b); endfunction
+    export "DPI-C" asdf = function f3;
+endmodule
+
+module m2;
+    function int f4(longint a, output b, input c); endfunction
+    export "DPI-C" asdf = function f4;
+endmodule
+
+module m3;
+    function int f5(int a, input b); endfunction
+    export "DPI-C" asdf = function f5;
+endmodule
+
+module m4;
+    function int asdf(int a, output b); endfunction
+    export "DPI-C" function asdf;
+
+    function int blah(int a, output b); endfunction
+    export "DPI-C" asdf = function blah;
+endmodule
+)");
+
+    Compilation compilation;
+    compilation.addSyntaxTree(tree);
+
+    auto& diags = compilation.getAllDiagnostics();
+    REQUIRE(diags.size() == 6);
+    CHECK(diags[0].code == diag::DPISignatureMismatch);
+    CHECK(diags[1].code == diag::DPISignatureMismatch);
+    CHECK(diags[2].code == diag::DPISignatureMismatch);
+    CHECK(diags[3].code == diag::DPISignatureMismatch);
+    CHECK(diags[4].code == diag::DPISignatureMismatch);
+    CHECK(diags[5].code == diag::DPIExportDuplicateCId);
 }
 
 TEST_CASE("Non-const subroutine check failures") {