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.

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

fix Issue 15579 - extern(C++) interfaces/multiple-inheritance #5361

Merged
merged 1 commit into from
Jan 22, 2016
Merged

fix Issue 15579 - extern(C++) interfaces/multiple-inheritance #5361

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
5 changes: 3 additions & 2 deletions src/dclass.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1156,6 +1156,9 @@ public:
structsize = Target.ptrsize * 2; // allow room for __vptr and __monitor
}

// Add vptr's for any interfaces implemented by this class
structsize += setBaseInterfaceOffsets(structsize);

Copy link
Contributor

Choose a reason for hiding this comment

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

This change affects extern (D) class layout, not only for extern (C++). Test case is:

extern (D) //extern (C++)
{
    class Base
    {
        ~this() {}
        size_t x = 4;
    }

    interface Interface
    {
        int Method();
    }

    class Derived : Base, Interface
    {
        size_t y = 5;
        int Method() { return 3; }
    }
}

void main()
{
    Derived d;                  // dmd 2.069             master
    pragma(msg, d.x.offsetof);  // (void*).sizeof * 2
    pragma(msg, d.y.offsetof);  // (void*).sizeof * 3 -> 4
}

Is this intentional? If so, we should update ABI page.

Copy link
Contributor

Choose a reason for hiding this comment

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

I opened a regression issue: https://issues.dlang.org/show_bug.cgi?id=15618

uint offset = structsize;
for (size_t i = 0; i < members.dim; i++)
{
Expand All @@ -1165,8 +1168,6 @@ public:
if (sizeok == SIZEOKfwd)
return;

// Add vptr's for any interfaces implemented by this class
structsize += setBaseInterfaceOffsets(structsize);
sizeok = SIZEOKdone;

// Calculate fields[i].overlapped
Expand Down
43 changes: 21 additions & 22 deletions src/e2ir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3970,28 +3970,6 @@ elem *toElem(Expression *e, IRState *irs)
ClassDeclaration *cdfrom = tfrom->isClassHandle();
ClassDeclaration *cdto = t->isClassHandle();

if (cdfrom->cpp)
{
if (cdto->cpp)
{
/* Casting from a C++ interface to a C++ interface
* is always a 'paint' operation
*/
goto Lret; // no-op
}

/* Casting from a C++ interface to a class
* always results in null because there is no runtime
* information available to do it.
*
* Casting from a C++ interface to a non-C++ interface
* always results in null because there's no way one
* can be derived from the other.
*/
e = el_bin(OPcomma, TYnptr, e, el_long(TYnptr, 0));
goto Lret;
}

int offset;
if (cdto->isBaseOf(cdfrom, &offset) && offset != OFFSET_RUNTIME)
{
Expand Down Expand Up @@ -4024,6 +4002,27 @@ elem *toElem(Expression *e, IRState *irs)
// Casting from derived class to base class is a no-op
}
}
else if (cdfrom->cpp)
{
if (cdto->cpp)
{
/* Casting from a C++ interface to a C++ interface
* is always a 'paint' operation
*/
goto Lret; // no-op
}

/* Casting from a C++ interface to a class
* always results in null because there is no runtime
* information available to do it.
*
* Casting from a C++ interface to a non-C++ interface
* always results in null because there's no way one
* can be derived from the other.
*/
e = el_bin(OPcomma, TYnptr, e, el_long(TYnptr, 0));
goto Lret;
}
else
{
/* The offset from cdfrom => cdto can only be determined at runtime.
Expand Down
108 changes: 60 additions & 48 deletions src/todt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,13 +640,25 @@ dt_t **cpp_type_info_ptr_toDt(ClassDeclaration *cd, dt_t **pdt)
* Put out initializers of ad->fields[].
* Although this is consistent with the elements[] version, we
* have to use this optimized version to reduce memory footprint.
* Params:
* ad = aggregate with members
* pdt = tail of initializer list
* concreteType = structs: null, classes: top level class
* Returns:
* updated tail of dt_t list
*/
dt_t **membersToDt(AggregateDeclaration *ad, dt_t **pdt,
ClassDeclaration *concreteType)
{
//printf("membersToDt(ad = '%s')\n", ad->toChars());
ClassDeclaration *cd = ad->isClassDeclaration();

/* Order:
* { base class } or { __vptr, __monitor)
* interfaces
* fields
*/

unsigned offset;
if (cd)
{
Expand All @@ -666,6 +678,30 @@ dt_t **membersToDt(AggregateDeclaration *ad, dt_t **pdt,
else
offset = 0;

if (cd)
{
// Interface vptr initializations
toSymbol(cd); // define csym

for (size_t i = 0; i < cd->vtblInterfaces->dim; i++)
{
BaseClass *b = (*cd->vtblInterfaces)[i];
for (ClassDeclaration *cd2 = concreteType; 1; cd2 = cd2->baseClass)
{
assert(cd2);
unsigned csymoffset = baseVtblOffset(cd2, b);
if (csymoffset != ~0)
{
if (offset < b->offset)
pdt = dtnzeros(pdt, b->offset - offset);
pdt = dtxoff(pdt, toSymbol(cd2), csymoffset);
break;
}
}
offset = b->offset + Target::ptrsize;
}
}

for (size_t i = 0; i < ad->fields.dim; i++)
{
if (ad->fields[i]->_init && ad->fields[i]->_init->isVoidInitializer())
Expand Down Expand Up @@ -730,30 +766,6 @@ dt_t **membersToDt(AggregateDeclaration *ad, dt_t **pdt,
offset = vd->offset + vd->type->size();
}

if (cd)
{
// Interface vptr initializations
toSymbol(cd); // define csym

for (size_t i = 0; i < cd->vtblInterfaces->dim; i++)
{
BaseClass *b = (*cd->vtblInterfaces)[i];
for (ClassDeclaration *cd2 = concreteType; 1; cd2 = cd2->baseClass)
{
assert(cd2);
unsigned csymoffset = baseVtblOffset(cd2, b);
if (csymoffset != ~0)
{
if (offset < b->offset)
pdt = dtnzeros(pdt, b->offset - offset);
pdt = dtxoff(pdt, toSymbol(cd2), csymoffset);
break;
}
}
offset = b->offset + Target::ptrsize;
}
}

if (offset < ad->structsize)
pdt = dtnzeros(pdt, ad->structsize - offset);

Expand Down Expand Up @@ -792,6 +804,30 @@ dt_t **membersToDt(AggregateDeclaration *ad, dt_t **pdt,
else
offset = 0;

if (cd)
{
// Interface vptr initializations
toSymbol(cd); // define csym

for (size_t i = 0; i < cd->vtblInterfaces->dim; i++)
{
BaseClass *b = (*cd->vtblInterfaces)[i];
for (ClassDeclaration *cd2 = concreteType; 1; cd2 = cd2->baseClass)
{
assert(cd2);
unsigned csymoffset = baseVtblOffset(cd2, b);
if (csymoffset != ~0)
{
if (offset < b->offset)
pdt = dtnzeros(pdt, b->offset - offset);
pdt = dtxoff(pdt, toSymbol(cd2), csymoffset);
break;
}
}
offset = b->offset + Target::ptrsize;
}
}

assert(firstFieldIndex <= elements->dim &&
firstFieldIndex + ad->fields.dim <= elements->dim);
for (size_t i = 0; i < ad->fields.dim; i++)
Expand Down Expand Up @@ -835,30 +871,6 @@ dt_t **membersToDt(AggregateDeclaration *ad, dt_t **pdt,
offset = vd->offset + vd->type->size();
}

if (cd)
{
// Interface vptr initializations
toSymbol(cd); // define csym

for (size_t i = 0; i < cd->vtblInterfaces->dim; i++)
{
BaseClass *b = (*cd->vtblInterfaces)[i];
for (ClassDeclaration *cd2 = concreteType; 1; cd2 = cd2->baseClass)
{
assert(cd2);
unsigned csymoffset = baseVtblOffset(cd2, b);
if (csymoffset != ~0)
{
if (offset < b->offset)
pdt = dtnzeros(pdt, b->offset - offset);
pdt = dtxoff(pdt, toSymbol(cd2), csymoffset);
break;
}
}
offset = b->offset + Target::ptrsize;
}
}

if (offset < ad->structsize)
pdt = dtnzeros(pdt, ad->structsize - offset);

Expand Down
56 changes: 56 additions & 0 deletions test/runnable/cppa.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -982,6 +982,61 @@ void testeh3()
}
}

/****************************************/
// 15579

extern (C++)
{
class Base
{
//~this() {}
void based() { }
ubyte x = 4;
}

interface Interface
{
int MethodCPP();
int MethodD();
}

class Derived : Base, Interface
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess, D compiler doesn't add pointer to the typeid(Derived) into its vtbl.
Maybe we should add pointer to the C++-generated RTTI as external symbol? Now we know how to mangle C++ type_info and we know that Derived defined in C++ code.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's beyond the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. I'll try to do it later.

{
short y = 5;
int MethodCPP();
int MethodD() { return 3; }
}

Derived cppfoo(Derived);
Interface cppfooi(Interface);
}

void test15579()
{
Derived d = new Derived();
assert(d.x == 4);
assert(d.y == 5);
assert(d.MethodD() == 3);
assert(d.MethodCPP() == 30);

d = cppfoo(d);
assert(d.x == 7);
assert(d.y == 8);
version (Win64)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should write version (CRuntime_Microsoft) instead of version (Win64).
IIRC, dmd supports 32-bit coff and we may compile 32-bit code for visual studio.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's just for the moment until I get it fixed.

{
// needs more work
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this suggest this is not yet compatible with MSC-Win64?

Copy link
Contributor

Choose a reason for hiding this comment

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

Walter wrote that he want implement MSC part it another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this suggest this is not yet compatible with MSC-Win64?

That's right. Win32 and Posix systems work fine, but VC++ is doing something different with the vtbl[]s. Grrr. The current fix is not wrong, it is still necessary for Win64. It's just that that particular test case still fails (before the PR, all of them failed).

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I'll wait for the next patch to try it out.

}
else
{
assert(d.MethodD() == 3);
assert(d.MethodCPP() == 30);
}
printf("d = %p, i = %p\n", d, cast(Interface)d);
Interface i = cppfooi(d);
assert(i.MethodD() == 3);
assert(i.MethodCPP() == 30);
}

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

void main()
Expand Down Expand Up @@ -1018,6 +1073,7 @@ void main()
testeh();
testeh2();
testeh3();
test15579();

printf("Success\n");
}
62 changes: 62 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 @@ -578,4 +578,66 @@ void throwle()
#endif

/******************************************/
// 15579

class Base
{
public:
//virtual ~Base() {}
virtual void base();
unsigned char x;
};

class Interface
{
public:
virtual int MethodCPP() = 0;
virtual int MethodD() = 0;
};

class Derived : public Base, public Interface
{
public:
Derived();
short y;
int MethodCPP();
#if _WIN32 || _WIN64
int MethodD();
#else
int MethodD() { return 3; } // need def or vtbl[] is not generated
#endif
};

void Base::base() { }
int Derived::MethodCPP() { return 30; }
Derived::Derived() { }


Derived *cppfoo(Derived *d)
{
assert(d->x == 4);
assert(d->y == 5);
assert(d->MethodD() == 3);
assert(d->MethodCPP() == 30);

d = new Derived();
d->x = 7;
d->y = 8;
assert(d->MethodD() == 3);
assert(d->MethodCPP() == 30);
return d;
}

Interface *cppfooi(Interface *i)
{
printf("i = %p\n", i);
assert(i->MethodD() == 3);
assert(i->MethodCPP() == 30);
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if we write (dynamic_cast<Derived*>(i))->y here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will fail because D does not generate C++ typeinfo.


Derived *d = new Derived();
printf("d = %p, i = %p\n", d, (Interface *)d);
return d;
}

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