Skip to content
This repository has been archived by the owner on Jun 20, 2019. It is now read-only.

Remove IN_GCC code for xopCmp/xopEquals in compiler and library #465

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Remove IN_GCC code for xopCmp/xopEquals in compiler and library #465

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
12 changes: 4 additions & 8 deletions gcc/d/dfrontend/clone.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ FuncDeclaration *buildXopEquals(StructDeclaration *sd, Scope *sc)
Parameters *parameters = new Parameters;
parameters->push(new Parameter(STCref | STCconst, sd->type, Id::p, NULL));
parameters->push(new Parameter(STCref | STCconst, sd->type, Id::q, NULL));
TypeFunction *tf = new TypeFunction(parameters, Type::tbool, 0, LINKd);
TypeFunction *tf = new TypeFunction(parameters, Type::tbool, 0, LINKc);

Identifier *id = Id::xopEquals;
FuncDeclaration *fop = new FuncDeclaration(declLoc, Loc(), id, STCstatic, tf);
Expand All @@ -551,7 +551,7 @@ FuncDeclaration *buildXopEquals(StructDeclaration *sd, Scope *sc)
unsigned errors = global.startGagging(); // Do not report errors
Scope *sc2 = sc->push();
sc2->stc = 0;
sc2->linkage = LINKd;
sc2->linkage = LINKc;

fop->semantic(sc2);
fop->semantic2(sc2);
Expand Down Expand Up @@ -657,25 +657,21 @@ FuncDeclaration *buildXopCmp(StructDeclaration *sd, Scope *sc)
Parameters *parameters = new Parameters;
parameters->push(new Parameter(STCref | STCconst, sd->type, Id::p, NULL));
parameters->push(new Parameter(STCref | STCconst, sd->type, Id::q, NULL));
TypeFunction *tf = new TypeFunction(parameters, Type::tint32, 0, LINKd);
TypeFunction *tf = new TypeFunction(parameters, Type::tint32, 0, LINKc);

Identifier *id = Id::xopCmp;
FuncDeclaration *fop = new FuncDeclaration(declLoc, Loc(), id, STCstatic, tf);
fop->generated = true;
Expression *e1 = new IdentifierExp(loc, Id::p);
Expression *e2 = new IdentifierExp(loc, Id::q);
#ifdef IN_GCC
Expression *e = new CallExp(loc, new DotIdExp(loc, e1, Id::cmp), e2);
#else
Expression *e = new CallExp(loc, new DotIdExp(loc, e2, Id::cmp), e1);
#endif

fop->fbody = new ReturnStatement(loc, e);

unsigned errors = global.startGagging(); // Do not report errors
Scope *sc2 = sc->push();
sc2->stc = 0;
sc2->linkage = LINKd;
sc2->linkage = LINKc;

fop->semantic(sc2);
fop->semantic2(sc2);
Expand Down
53 changes: 53 additions & 0 deletions gcc/testsuite/gdc.test/runnable/test5854.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
struct S1
{
long x1, x2;

int opCmp(ref const S1 s) const
{
if (x2 - s.x2 != 0)
return x2 < s.x2 ? -1 : 1;

return x1 < s.x1 ? -1 : x1 == s.x1 ? 0 : 1;
}
}

struct S2
{
long x1, x2;

int opCmp(in S2 s) const
{
if (x2 - s.x2 != 0)
return x2 < s.x2 ? -1 : 1;

return x1 < s.x1 ? -1 : x1 == s.x1 ? 0 : 1;
}
}

extern (C) int structcmp(T)(scope T a, scope T b)
{
extern (C) int cmp(scope const void* p1, scope const void* p2, scope void* ti)
{
return (cast(TypeInfo)ti).compare(p1, p2);
}
return cmp(&a, &b, cast(void*)typeid(T));
}

void test5854()
{
alias Tuple(T...) = T;

foreach (T; Tuple!(S1, S2))
{
assert(T(1, 3).structcmp(T(4, 4)) == -1);
assert(T(1, 3).structcmp(T(4, 3)) == -1);
assert(T(4, 3).structcmp(T(4, 3)) == 0);
assert(T(5, 3).structcmp(T(4, 3)) == 1);
assert(T(5, 4).structcmp(T(4, 3)) == 1);
}
}

void main()
{
test5854();
}
35 changes: 17 additions & 18 deletions libphobos/libdruntime/object.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1145,12 +1145,7 @@ class TypeInfo_Struct : TypeInfo
return false;
else if (xopEquals)
{
version(GNU)
{ // BUG: GDC and DMD use different calling conventions
return (*xopEquals)(p2, p1);
}
else
return (*xopEquals)(p1, p2);
return (*xopEquals)(p1, p2);
}
else if (p1 == p2)
return true;
Expand All @@ -1171,14 +1166,7 @@ class TypeInfo_Struct : TypeInfo
if (!p2)
return true;
else if (xopCmp)
{
version(GNU)
{ // BUG: GDC and DMD use different calling conventions
return (*xopCmp)(p1, p2);
}
else
return (*xopCmp)(p2, p1);
}
return (*xopCmp)(p1, p2);
else
// BUG: relies on the GC not moving objects
return memcmp(p1, p2, initializer().length);
Expand Down Expand Up @@ -1225,10 +1213,21 @@ class TypeInfo_Struct : TypeInfo

@safe pure nothrow
{
size_t function(in void*) xtoHash;
bool function(in void*, in void*) xopEquals;
int function(in void*, in void*) xopCmp;
string function(in void*) xtoString;
size_t function(in void*) xtoHash;
/* The xopEquals and xopCmp function pointers usually point to the struct's
* opEquals and opCmp methods. If the method doesn't take its single
* argument by reference, the front-end injects a static __xopEquals/
* __xopCmp function (taking 2 arguments, lhs `p` and rhs `q`).
*
* In the method case, lhs `p` is the `this` argument and must be passed
* as first argument before rhs `q`.
* Enforce this arguments order by marking the pointed-to functions as
* using the C calling convention, for which the arguments are never
* reversed (contrary to `extern (D)`).
*/
extern (C) bool function(in void*, in void*) xopEquals;
Copy link
Contributor

Choose a reason for hiding this comment

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

xopEquals is now in the global namespace. I wonder whether we should prefix all (new) extern(C) druntime functions with something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think the frontend cares about that. As it makes the call through a function pointer, so this only changes ABI of how parameters are expected to be passed.

I didn't check the code in clone.d, but when I spoke to Daniel about it, I recall him saying that it's just an abi hack. The compiler sees xopCmp as a C function internally, but reverses the arguments as its really calling a D method.

This reversing of arguments never worked for gdc.

...

However I should check something about this, as it appears that the test for xopCmp parameter order was removed or no longer uses the generated xopCmp call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you're right, I totally forgot that this is just a pointer to a generated function (which then obviously has a unique name) 👍
Does the frontend emit these xopCmp functions as extern(C) or extern(D)?

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 is just what I've pushed a test for. The __xopCmp function is marked as LINKd at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although, now I think about it, it doesn't really matter for us as LINKd and LINKc are the same...

extern (C) int function(in void*, in void*) xopCmp;
string function(in void*) xtoString;

enum StructFlags : uint
{
Expand Down