Skip to content

fix Issue 18966 - extern(C++) constructor should match C++ semantics … #8362

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.

Sign up for GitHub

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

Merged
dlang-bot merged 1 commit into from
Jun 17, 2018
Merged

fix Issue 18966 - extern(C++) constructor should match C++ semantics … #8362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/dmd/aggregate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ class ClassDeclaration : public AggregateDeclaration
void addLocalClass(ClassDeclarations *);

// Back end
Symbol *vtblsym;
Dsymbol *vtblsym;

ClassDeclaration *isClassDeclaration() { return (ClassDeclaration *)this; }
void accept(Visitor *v) { v->visit(this); }
Expand Down
17 changes: 16 additions & 1 deletion src/dmd/dclass.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -973,7 +973,22 @@ extern (C++) class ClassDeclaration : AggregateDeclaration
}

// Back end
Symbol* vtblsym;
Dsymbol vtblsym;

final Dsymbol vtblSymbol()
{
if (!vtblsym)
{
auto vtype = Type.tvoidptr.immutableOf();
auto var = new VarDeclaration(loc, vtype, Identifier.idPool("__vtbl"), null, STC.immutable_ | STC.static_);
var.addMember(null, this);
var.isdataseg = 1;
var.linkage = LINK.d;
var.semanticRun = PASS.semanticdone; // no more semantic wanted
vtblsym = var;
}
return vtblsym;
}

override final inout(ClassDeclaration) isClassDeclaration() inout
{
Expand Down
29 changes: 28 additions & 1 deletion src/dmd/expressionsem.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3105,6 +3105,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
return f;
}

bool isSuper = false;
if (exp.e1.op == TOK.dotVariable && t1.ty == Tfunction || exp.e1.op == TOK.dotTemplateDeclaration)
{
UnaExp ue = cast(UnaExp)exp.e1;
Expand Down Expand Up @@ -3250,7 +3251,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
auto ad = sc.func ? sc.func.isThis() : null;
auto cd = ad ? ad.isClassDeclaration() : null;

const bool isSuper = exp.e1.op == TOK.super_;
isSuper = exp.e1.op == TOK.super_;
if (isSuper)
{
// Base class constructor call
Expand Down Expand Up @@ -3588,6 +3589,32 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
}

result = Expression.combine(argprefix, exp);

if (isSuper)
{
auto ad = sc.func ? sc.func.isThis() : null;
auto cd = ad ? ad.isClassDeclaration() : null;
if (cd && cd.classKind == ClassKind.cpp)
{
// 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)
Copy link
Contributor

@kinke kinke Aug 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This vtbl assignment cannot be interpreted at compile-time, see https://forum.dlang.org/thread/ghusgzuqpcskhwzmlwbh@forum.dlang.org. Happening here:

pointer cast from cpp.Y to immutable(void*)** is not supported at compile time

Copy link
Contributor

@TurkeyMan TurkeyMan Aug 2, 2018

Choose a reason for hiding this comment

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

How does CTFE assign vtables?

Copy link
Contributor

@TurkeyMan TurkeyMan Aug 2, 2018

Choose a reason for hiding this comment

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

Is this actually an issue? Any class with an opaque constructor (ie, is defined in C++) can't possibly CTFE...?

Copy link
Contributor

@kinke kinke Aug 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, if there's an external ctor, CTFE will fail anyway (untested :]). If they're all in D, it used to work before, so it's clearly a regression, and resetting the vptr can probably be safely skipped during CTFE (something like __ctfe || (this.__vptr = __vtbl)).

Copy link
Contributor

@kinke kinke Aug 2, 2018

Choose a reason for hiding this comment

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

=> #8533

Copy link
Contributor

@TurkeyMan TurkeyMan Aug 3, 2018

Choose a reason for hiding this comment

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

Yeah, if that logic is only performed when the base constructor is extern, then it should still be fine.

__gshared int superid = 0;
char[20] buf;
sprintf(buf.ptr, "__super%d", superid++);
auto tmp = copyToTemp(0, buf.ptr, result);
Loc loc = exp.loc;
Expression tmpdecl = new DeclarationExp(loc, tmp);

auto dse = new DsymbolExp(loc, cd.vtblSymbol());
auto ase = new AddrExp(loc, dse);
auto pte = new DotIdExp(loc, new ThisExp(loc), Id.__vptr);
auto ate = new AssignExp(loc, pte, ase);

Expression e = new CommaExp(loc, new CommaExp(loc, tmpdecl, ate), new VarExp(loc, tmp));
result = e.expressionSemantic(sc);
}
}
}

override void visit(DeclarationExp e)
Expand Down
8 changes: 5 additions & 3 deletions src/dmd/tocsym.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,7 +549,7 @@ Classsym *fake_classsym(Identifier id)

Symbol *toVtblSymbol(ClassDeclaration cd)
{
if (!cd.vtblsym)
if (!cd.vtblsym || !cd.vtblsym.csym)
{
if (!cd.csym)
toSymbol(cd);
Expand All @@ -559,9 +559,11 @@ Symbol *toVtblSymbol(ClassDeclaration cd)
auto s = toSymbolX(cd, "__vtbl", SCextern, t, "Z");
s.Sflags |= SFLnodebug;
s.Sfl = FLextern;
cd.vtblsym = s;

auto vtbl = cd.vtblSymbol();
vtbl.csym = s;
}
return cd.vtblsym;
return cd.vtblsym.csym;
}

/**********************************
Expand Down
14 changes: 7 additions & 7 deletions src/dmd/toobj.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,13 +419,13 @@ void toObjFile(Dsymbol ds, bool multiobj)
*/
dtbv.size(0);
}
cd.vtblsym.Sdt = dtbv.finish();
cd.vtblsym.Sclass = scclass;
cd.vtblsym.Sfl = FLdata;
out_readonly(cd.vtblsym);
outdata(cd.vtblsym);
cd.vtblsym.csym.Sdt = dtbv.finish();
cd.vtblsym.csym.Sclass = scclass;
cd.vtblsym.csym.Sfl = FLdata;
out_readonly(cd.vtblsym.csym);
outdata(cd.vtblsym.csym);
if (cd.isExport())
objmod.export_symbol(cd.vtblsym,0);
objmod.export_symbol(cd.vtblsym.csym,0);
}

override void visit(InterfaceDeclaration id)
Expand Down Expand Up @@ -1264,7 +1264,7 @@ private void genClassInfoForClass(ClassDeclaration cd, Symbol* sinit)
// vtbl[]
dtb.size(cd.vtbl.dim);
if (cd.vtbl.dim)
dtb.xoff(cd.vtblsym, 0, TYnptr);
dtb.xoff(cd.vtblsym.csym, 0, TYnptr);
else
dtb.size(0);

Expand Down
48 changes: 48 additions & 0 deletions test/runnable/cppa.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1441,6 +1441,53 @@ void test18928()
assert(s.x == 5);
}

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

extern(C++):
class Base18966
{
this() @safe nothrow;
~this();
void vf();
int x;
}

class Derived18966 : Base18966
{
override void vf() { x = 200; }
}

class Explicit18966 : Base18966
{
this() @safe { super(); }
override void vf() { x = 250; }
}

class Implicit18966 : Base18966
{
this() nothrow {}
override void vf() { x = 300; }
}

void test18966()
{
Derived18966 d = new Derived18966;
assert(d.x == 10);
d.vf();
assert(d.x == 200);

Explicit18966 e = new Explicit18966;
assert(e.x == 10);
e.vf();
assert(e.x == 250);

Implicit18966 i = new Implicit18966;
assert(i.x == 10);
i.vf();
assert(i.x == 300);
}

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

void main()
Expand Down Expand Up @@ -1488,6 +1535,7 @@ void main()
test15589();
test15589b();
test18928();
test18966();

printf("Success\n");
}
9 changes: 9 additions & 0 deletions test/runnable/extra-files/cppb.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,3 +918,12 @@ CC18928* newCC18928()
{
return new CC18928();
}

/****************************************/
// https://issues.dlang.org/show_bug.cgi?id=18966
Base18966::Base18966() { x = 10; }
Base18966::~Base18966() {}
void Base18966::vf()
{
x = 100;
}
9 changes: 9 additions & 0 deletions test/runnable/extra-files/cppb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,12 @@ struct Cpp15589Struct
~Cpp15589Struct();
int s;
};

class Base18966
{
public:
Base18966();
virtual ~Base18966();
virtual void vf();
int x;
};