Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stable] Fix Issue 19134 - [C++] static const y = new Derived(); ->pointer cast from const(Derived) to immutable(void*)** is not supported at compile time #8533

Merged
merged 1 commit into from
Aug 24, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions src/dmd/expressionsem.d
Original file line number Diff line number Diff line change
Expand Up @@ -4547,24 +4547,25 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
{
auto ad = sc.func ? sc.func.isThis() : null;
auto cd = ad ? ad.isClassDeclaration() : null;
if (cd && cd.classKind == ClassKind.cpp)
if (cd && cd.classKind == ClassKind.cpp && exp.f && !exp.f.fbody)
{
// if super is defined in C++, it sets the vtable pointer to the base class
// so we have to rewrite it, but still return 'this' from super() call:
// (auto tmp = super(), this.__vptr = __vtbl, tmp)
__gshared int superid = 0;
char[20] buf;
sprintf(buf.ptr, "__super%d", superid++);
auto tmp = copyToTemp(0, buf.ptr, result);
// so we have to restore it, but still return 'this' from super() call:
// (auto __vptrTmp = this.__vptr, auto __superTmp = super()), (this.__vptr = __vptrTmp, __superTmp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might have missed some comments, but is saving and restoring the vptr good enough? If the class instance is created in C++, the vptr might be garbage. Granted, other fields might not be initialized, too, but that can be solved in user code.

Can the CTFE issue be solved by preassigning some appropriate type to one of the expressions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it's fine for now and reverts to previous D vptr semantics in extern(C++) ctors. IIRC, I didn't find anything about this breaking change mentioned in the changelog.

but that can be solved in user code.

I don't see a good way of doing that (and don't want to burden the user with these details), and so suggested #8533 (comment) for fixing this preblit-dependency properly in the long run. [The implicit default C++ ctor would be useful for extern(C++) structs too btw.]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of initializing the fields explicitly with some foreach over C.tupleof, but that is actually putting the burden on the user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, it's fine for now and reverts to previous D vptr semantics in extern(C++) ctors.

Ok, allocating a class defined in D from C++ is still trouble anyway, and solving this for classes that have super() calls into C++-code is a very special case, only.

Loc loc = exp.loc;
Expression tmpdecl = new DeclarationExp(loc, tmp);

auto dse = new DsymbolExp(loc, cd.vtblSymbol());
auto ase = new AddrExp(loc, new IndexExp(loc, dse, new IntegerExp(loc, 0, Type.tsize_t)));
auto pte = new DotIdExp(loc, new ThisExp(loc), Id.__vptr);
auto ate = new AssignExp(loc, pte, ase);
auto vptr = new DotIdExp(loc, new ThisExp(loc), Id.__vptr);
auto vptrTmpDecl = copyToTemp(0, "__vptrTmp", vptr);
auto declareVptrTmp = new DeclarationExp(loc, vptrTmpDecl);

Expression e = new CommaExp(loc, new CommaExp(loc, tmpdecl, ate), new VarExp(loc, tmp));
auto superTmpDecl = copyToTemp(0, "__superTmp", result);
auto declareSuperTmp = new DeclarationExp(loc, superTmpDecl);

auto declareTmps = new CommaExp(loc, declareVptrTmp, declareSuperTmp);

auto restoreVptr = new AssignExp(loc, vptr.syntaxCopy(), new VarExp(loc, vptrTmpDecl));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if vptr.syntaxCopy() is required or whether vptr could be reused directly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better safe than sorry, using the same expression instance twice might cause issues if the AST gets modified by lowerings.


Expression e = new CommaExp(loc, declareTmps, new CommaExp(loc, restoreVptr, new VarExp(loc, superTmpDecl)));
Copy link
Contributor Author

@kinke kinke Aug 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using Expression.combine(a, b, c, d) here, creating equivalent ((a, b), (c, d)), all hell breaks loose, requiring additional expressionSemantic() calls; combine(lhs, rhs) additionally sets the new CommaExp.type to the rhs type, which seems to make expressionSemantic() not descend to both children (which then leads to forward-referencing errors later on...).

result = e.expressionSemantic(sc);
}
}
Expand Down
81 changes: 81 additions & 0 deletions test/runnable/cppa.d
Original file line number Diff line number Diff line change
Expand Up @@ -1495,6 +1495,37 @@ class Implicit18966 : Base18966
override void vf() { x = 300; }
}

// test vptr in full ctor chain of mixed D/C++ class hierarchies

// TODO: Make this a D class and let C++ derive from it. This works on Windows,
// but results in linker errors on Posix due to extra base ctor (`C2`
// mangling) being called by the B ctor.
class A18966 // in C++
{
char[8] calledOverloads = 0;
int i;
this();
void foo();
}

class B18966 : A18966 // in C++
{
this();
override void foo();
}

class C18966 : B18966
{
this() { foo(); }
override void foo() { calledOverloads[i++] = 'C'; }
}

class D18966 : C18966
{
this() { foo(); }
override void foo() { calledOverloads[i++] = 'D'; }
}

void test18966()
{
Derived18966 d = new Derived18966;
Expand All @@ -1511,6 +1542,55 @@ void test18966()
assert(i.x == 10);
i.vf();
assert(i.x == 300);

// TODO: Allocating + constructing a C++ class with the D GC is not
// supported on Posix. The returned pointer (probably from C++ ctor)
// seems to be an offset and not the actual object address.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some platforms don't return the this pointer from the C++ constructor, see scopeAllocCpp above for a workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already fixed this for LDC, by simply ignoring the returned pointer, see ldc-developers/ldc@25d7a79.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess dmd should do the same.

version (Windows)
{
auto a = new A18966;
assert(a.calledOverloads[0..2] == "A\0");

auto b = new B18966;
assert(b.calledOverloads[0..3] == "AB\0");
}

auto c = new C18966;
assert(c.calledOverloads[0..4] == "ABC\0");

auto d2 = new D18966;
// note: the vptr semantics in ctors of extern(C++) classes may be revised (to "ABCD")
assert(d2.calledOverloads[0..5] == "ABDD\0");
}

/****************************************/

// https://issues.dlang.org/show_bug.cgi?id=19134

class Base19134
{
int a = 123;
this() { a += 42; }
int foo() const { return a; }
}

class Derived19134 : Base19134
{
int b = 666;
this()
{
a *= 2;
b -= 6;
}
override int foo() const { return b; }
}

void test19134()
{
static const d = new Derived19134;
assert(d.a == (123 + 42) * 2);
assert(d.b == 666 - 6);
assert(d.foo() == 660);
}

/****************************************/
Expand Down Expand Up @@ -1562,6 +1642,7 @@ void main()
test18928();
test18953();
test18966();
test19134();

printf("Success\n");
}
6 changes: 6 additions & 0 deletions test/runnable/extra-files/cppb.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -927,3 +927,9 @@ void Base18966::vf()
{
x = 100;
}

A18966::A18966() : calledOverloads(/*zero-init*/), i(0) { foo(); }
void A18966::foo() { calledOverloads[i++] = 'A'; }

B18966::B18966() { foo(); }
void B18966::foo() { calledOverloads[i++] = 'B'; }
16 changes: 16 additions & 0 deletions test/runnable/extra-files/cppb.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,19 @@ class Base18966
virtual void vf();
int x;
};

class A18966
{
public:
char calledOverloads[8];
int i;
A18966();
virtual void foo();
};

class B18966 : public A18966
{
public:
B18966();
void foo() /*override*/;
};