Make C++ structs go through LDC's toArgTypes() which follows the same…

… ABIs that Clang follows, remove LangPlugin::passAggregateArgumentByRef(). toArgTypes should be able to handle C++ non-dynamic records, while C++ dynamic classes are never passed through registers during function calls. Fixes issue #121. The decision to pass a record in registers remains fragile regarding the expectations of C++ compilers. This is only an issue for extern(C++) functions defined in D that need to be passed to C++ code however. Also backport the two changes by 18a4606 to prevent byval argument copies without copy ctor calls.
Syniurge · Feb 6, 2020 · 1821f18 · 1821f18
1 parent 7c89525
commit 1821f18
Show file tree

Hide file tree

Showing 11 changed files with 206 additions and 38 deletions.
diff --git a/dmd/argtypes.d b/dmd/argtypes.d
@@ -515,10 +515,10 @@ extern (C++) TypeTuple toArgTypes(Type t)
 
         override void visit(TypeClass t)
         {
-            if (t.byRef()) // CALYPSO
+            if (t.byRef())
                 result = new TypeTuple(Type.tvoidptr);
             else
-                result = new TypeTuple(); // pass on stack (TODO: follow the struct ways..)
+                result = new TypeTuple(); // CALYPSO NOTE: for both Itanium and Microsoft ABI, dynamic classes are never passed through registers
         }
     }
 

diff --git a/dmd/argtypes_sysv_x64.d b/dmd/argtypes_sysv_x64.d
@@ -257,9 +257,12 @@ extern (C++) final class ToClassesVisitor : Visitor
         return one(Class.integer);
     }
 
-    override void visit(TypeClass)
+    override void visit(TypeClass t)
     {
-        return one(Class.integer);
+        if (t.byRef())
+            return one(Class.integer);
+        else
+            return memory(); // CALYPSO NOTE: for both Itanium and Microsoft ABI, dynamic classes are never passed through registers
     }
 
     override void visit(TypeDArray)

diff --git a/dmd/cpp/calypso.h b/dmd/cpp/calypso.h
@@ -229,7 +229,6 @@ class LangPlugin final : public ::LangPlugin, public ::ForeignCodeGen
 
     LLType *toType(Type *t) override;
     llvm::FunctionType *toFunctionType(::FuncDeclaration *fdecl) override;
-    bool passAggregateArgumentByRef(AggregateDeclaration* ad) override;
     llvm::Type *IrTypeStructHijack(::StructDeclaration *sd) override;
 
     llvm::Constant *createInitializerConstant(IrAggr *irAggr,

diff --git a/dmd/cpp/cppaggregate.cpp b/dmd/cpp/cppaggregate.cpp
@@ -378,6 +378,19 @@ void ad_determineSize(AggTy *ad)
     auto CRD = dyn_cast<clang::CXXRecordDecl>(ad->RD);
     if (!CRD || (CRD->ctor_begin() == CRD->ctor_end() && !CRD->hasNonTrivialDefaultConstructor()))
         ad->zeroInit = true;
+
+    if (auto sd = ad->isStructDeclaration())
+    {
+        auto tt = target.toArgTypes(sd->type);
+        size_t dim = tt ? tt->arguments->dim : 0;
+        if (dim >= 1)
+        {
+            assert(dim <= 2);
+            sd->arg1type = (*tt->arguments)[0]->type;
+            if (dim == 2)
+                sd->arg2type = (*tt->arguments)[1]->type;
+        }
+    }
 }
 
 // NOTE: size() gets called to "determine fields", but shouldn't the two be separate?

diff --git a/gen/abi-x86-64.cpp b/gen/abi-x86-64.cpp
@@ -97,7 +97,7 @@ LLType *getAbiType(Type *ty) {
 }
 
 bool passByVal(Type *ty) {
-  TypeTuple *argTypes = target.toArgTypes(ty);
+  TypeTuple *argTypes = target.toArgTypes(ty); // CALYPSO NOTE: this is done for every function type without caching, shouldn't StructDeclaration.arg1type be used instead?
   if (!argTypes) {
     return false;
   }
@@ -263,7 +263,7 @@ bool X86_64TargetABI::returnInArg(TypeFunction *tf, bool) {
 
 bool X86_64TargetABI::passByVal(TypeFunction *tf, Type *t) {
   // indirectly by-value for extern(C++) functions and non-POD args
-  if (tf->linkage == LINKcpp && !isPOD(t))
+  if (/*tf->linkage == LINKcpp && */!isPOD(t)) // CALYPSO TODO this was fixed in LDC 1.19
     return false;
 
   return ::passByVal(t->toBasetype());
@@ -279,7 +279,7 @@ void X86_64TargetABI::rewriteArgument(IrFuncTy &fty, IrFuncTyArg &arg,
   Type *t = arg.type->toBasetype();
 
   // indirectly by-value for extern(C++) functions and non-POD args
-  if (fty.type->linkage == LINKcpp && !isPOD(t)) {
+  if (/*fty.type->linkage == LINKcpp && */!isPOD(t)) { // CALYPSO TODO see https://github.com/ldc-developers/ldc/commit/18a460683ff37b78a0fa027e2f1f8ac633fa7c00
     indirectByvalRewrite.applyTo(arg);
     if (regCount.int_regs > 0) {
       regCount.int_regs--;

diff --git a/gen/cgforeign.h b/gen/cgforeign.h
@@ -27,7 +27,6 @@ class ForeignCodeGen
 
     virtual LLType *toType(Type *t) = 0;
     virtual llvm::FunctionType *toFunctionType(FuncDeclaration *fdecl) = 0;
-    virtual bool passAggregateArgumentByRef(AggregateDeclaration* ad) = 0;
     virtual llvm::Type *IrTypeStructHijack(StructDeclaration *sd) = 0; // UGLY HACK
 
     virtual llvm::Constant *createInitializerConstant(IrAggr *irAggr,

diff --git a/gen/cpp/cpptoir.cpp b/gen/cpp/cpptoir.cpp
@@ -428,13 +428,6 @@ llvm::FunctionType *LangPlugin::toFunctionType(::FuncDeclaration *fdecl)
     return Resolved.Ty;
 }
 
-bool LangPlugin::passAggregateArgumentByRef(AggregateDeclaration* ad)
-{
-    auto RD = getRecordDecl(ad);
-    auto CRD = dyn_cast<clang::CXXRecordDecl>(RD);
-    return CRD && CGM->getCXXABI().getRecordArgABI(CRD) == clangCG::CGCXXABI::RAA_Indirect;
-}
-
 llvm::Type *LangPlugin::IrTypeStructHijack(::StructDeclaration *sd) // HACK but if we don't do this LLVM type comparisons will fail
 {
     if (sd->ident == id_cpp_member_ptr)

diff --git a/gen/functions.cpp b/gen/functions.cpp
@@ -172,12 +172,6 @@ llvm::FunctionType *DtoFunctionType(Type *type, IrFuncTy &irFty, Type *thistype,
     // Whether the parameter is passed by LLVM value or as a pointer to the
     // alloca/….
     bool passPointer = arg->storageClass & (STCref | STCout);
-
-    if (!passPointer && isAggregateValue(arg->type)) {  // CALYPSO
-        auto ad = getAggregateSym(arg->type);
-        if (auto lp = ad->langPlugin())
-            passPointer = lp->codegen()->passAggregateArgumentByRef(ad);
-    }
 
     Type *loweredDType = arg->type;
     llvm::AttrBuilder attrs;

diff --git a/tests/calypso/record_func_params.d b/tests/calypso/record_func_params.d
@@ -0,0 +1,117 @@
+// RUN: %ldc -cpp-cachedir=%t.cache -of %t %s
+// RUN: %t > %t.out
+// RUN: FileCheck %s < %t.out
+
+pragma(cppmap, "record_func_params.h");
+
+import (C++) *;
+import std.stdio : writeln;
+
+// TODO: check that extern(C++) functions match the expected C++ ABI?
+
+void testStructS(TestStructS s)
+{
+    writeln("StructS");
+    writeln("a = ", s.a);
+    writeln("");
+}
+
+void testStructM(TestStructM s)
+{
+    writeln("StructM");
+    writeln("a = ", s.a,
+            ", b = ", s.b,
+            ", c = ", s.c);
+    writeln("");
+}
+
+void testStructL(TestStructL s)
+{
+    writeln("StructL");
+    writeln("a = ", s.a,
+            ", b = ", s.b,
+            ", c = ", s.c,
+            ", d = ", s.d);
+    writeln("");
+}
+
+void testStructXL(TestStructXL s)
+{
+    writeln("StructXL");
+    writeln("a = ", s.a,
+            ", b = ", s.b,
+            ", c = ", s.c,
+            ", d = ", s.d,
+            ", e = ", s.e);
+    writeln("");
+}
+
+void testStructFloatS(TestStructFloatS s)
+{
+    writeln("StructFloatS");
+    writeln("a = ", s.a);
+    writeln("");
+}
+
+void testStructFloatM(TestStructFloatM s)
+{
+    writeln("StructFloatM");
+    writeln("a = ", s.a,
+            ", b = ", s.b,
+            ", c = ", s.c);
+    writeln("");
+}
+
+void testStructMixed(TestStructMixed s)
+{
+    writeln("StructMixed");
+    writeln("a = ", s.a,
+            ", b = ", s.b,
+            ", c = ", s.c,
+            ", d = ", s.d);
+    writeln("");
+}
+
+void testDC(DynamicClass c)
+{
+    c.makeMeDynamic();
+}
+
+void main()
+{
+    TestStructS s1 = { 38 };
+    testStructS(s1);
+    // CHECK: StructS
+    // CHECK: a = 38
+
+    TestStructM s2 = { 21, 735, 50000 };
+    testStructM(s2);
+    // CHECK: StructM
+    // CHECK: a = 21, b = 735, c = 50000
+
+    TestStructL s3 = { 85, 434, 90000, 222222 };
+    testStructL(s3);
+    // CHECK: StructL
+    // CHECK: a = 85, b = 434, c = 90000, d = 222222
+
+    TestStructXL s4 = { 13, 838, 10001, 747474, 8513214 };
+    testStructXL(s4);
+    // CHECK: StructXL
+    // CHECK: a = 13, b = 838, c = 10001, d = 747474, e = 8513214
+
+    TestStructFloatS s5 = { 40.0 };
+    testStructFloatS(s5);
+    // CHECK: StructFloatS
+    // CHECK: a = 40
+
+    TestStructFloatM s6 = { 78.0, 548.24, 963.14 };
+    testStructFloatM(s6);
+    // CHECK: StructFloatM
+    // CHECK: a = 78, b = 548.24, c = 963.14
+
+    TestStructMixed s7 = { 42.5555, 200000000000, 521.9, 500 };
+    testStructMixed(s7);
+    // CHECK: StructMixed
+    // CHECK: a = 42.5555, b = 200000000000, c = 521.9, d = 500
+}
+
diff --git a/tests/calypso/record_func_params.h b/tests/calypso/record_func_params.h
@@ -0,0 +1,53 @@
+struct TestStructS
+{
+    short a;
+};
+
+struct TestStructM
+{
+    int a;
+    int b;
+    int c;
+};
+
+struct TestStructL
+{
+    int a;
+    int b;
+    int c;
+    int d;
+};
+
+struct TestStructXL
+{
+    int a;
+    int b;
+    int c;
+    int d;
+    int e;
+};
+
+struct TestStructFloatS
+{
+    float a;
+};
+
+struct TestStructFloatM
+{
+    float a;
+    float b;
+    float c;
+};
+
+struct TestStructMixed
+{
+    double a;
+    long b;
+    float c;
+    int d;
+};
+
+class DynamicClass
+{
+    virtual void makeMeDynamic() {}
+};
diff --git a/tests/calypso/record_with_internal_pointers.h b/tests/calypso/record_with_internal_pointers.h
@@ -1,6 +1,5 @@
 #pragma once
 
-// #include <iostream>
 #include "assert.h"
 
 struct B {
@@ -16,25 +15,11 @@ struct B {
     assert(false); // ensures that the test case isn't calling the move ctor
   }
 
-  B &operator=(const B &a) {
-    initialize(a.x);
-    return *this;
-  }
-
-  B &operator=(B &&a) {
-    if (this == &a)
-      return *this;
-    initialize(a.x);
-    a.release();
-    return *this;
-  }
-
   B(int x) { initialize(x); }
 
   ~B() {}
 
   void release() {
-    // could release owned resources here
   }
 
   void initialize(int x) {
@@ -44,9 +29,21 @@ struct B {
   }
 
   void test() {
-//     std::cout << x << " " << y << " " << &x << " " << (&x == y) << std::endl;
     assert(&x == y);
   }
+
+  B &operator=(const B &a) {
+    initialize(a.x);
+    return *this;
+  }
+
+  B &operator=(B &&a) {
+    if (this == &a)
+      return *this;
+    initialize(a.x);
+    a.release();
+    return *this;
+  }
 };
 
 B foo() { return B(84); }