Skip to content

Commit

Permalink
fix Issue 15422 - associative array of nested struct - crash on usage
Browse files Browse the repository at this point in the history
If a struct is nested in a class, its `vthis` will be reference to the parent class, and struct default equality and hashing considers the parent class equality and hashing.
Otherwise, the `vthis` will be typed as void*, and pointer equality and hashing is merely used.
  • Loading branch information
9rnsr committed Feb 1, 2016
1 parent 8c60fe0 commit d789012
Show file tree
Hide file tree
Showing 5 changed files with 183 additions and 13 deletions.
2 changes: 2 additions & 0 deletions src/aggregate.d
Expand Up @@ -608,6 +608,8 @@ public:
return enclosing !is null;
}

/* Append vthis field (this.tupleof[$-1]) to make this aggregate type nested.
*/
final void makeNested()
{
if (enclosing) // if already nested
Expand Down
24 changes: 20 additions & 4 deletions src/clone.d
Expand Up @@ -224,7 +224,7 @@ extern (C++) FuncDeclaration buildOpAssign(StructDeclaration sd, Scope* sc)
}
else if (sd.dtor || sd.postblit)
{
/* Do swap this and rhs
/* Do swap this and rhs.
* __swap = this; this = s; __swap.dtor();
*/
//printf("\tswap copy\n");
Expand Down Expand Up @@ -254,7 +254,12 @@ extern (C++) FuncDeclaration buildOpAssign(StructDeclaration sd, Scope* sc)
}
else
{
/* Do memberwise copy
/* Do memberwise copy.
*
* If sd is a nested struct, its vthis field assignment is:
* 1. If it's nested in a class, it's a rebind of class reference.
* 2. If it's nested in a function or struct, it's an update of void*.
* In both cases, it will change the parent context.
*/
//printf("\tmemberwise copy\n");
for (size_t i = 0; i < sd.fields.dim; i++)
Expand Down Expand Up @@ -412,7 +417,7 @@ extern (C++) FuncDeclaration hasIdentityOpEquals(AggregateDeclaration ad, Scope*
* By fixing bugzilla 3789, opEquals is changed to be never implicitly generated.
* Now, struct objects comparison s1 == s2 is translated to:
* s1.tupleof == s2.tupleof
* to calculate structural equality. See EqualExp::semantic.
* to calculate structural equality. See EqualExp.op_overload.
*/
extern (C++) FuncDeclaration buildOpEquals(StructDeclaration sd, Scope* sc)
{
Expand Down Expand Up @@ -626,6 +631,7 @@ extern (C++) bool needToHash(StructDeclaration sd)
goto Lneed;
if (sd.isUnionDeclaration())
goto Ldontneed;

/* If any of the fields has an opEquals, then we
* need it too.
*/
Expand Down Expand Up @@ -697,7 +703,17 @@ extern (C++) FuncDeclaration buildXtoHash(StructDeclaration sd, Scope* sc)
auto tf = new TypeFunction(parameters, Type.thash_t, 0, LINKd, STCnothrow | STCtrusted);
Identifier id = Id.xtoHash;
auto fop = new FuncDeclaration(declLoc, Loc(), id, STCstatic, tf);
const(char)* code = "size_t h = 0;foreach (i, T; typeof(p.tupleof)) h += typeid(T).getHash(cast(const void*)&p.tupleof[i]);return h;";

/* Do memberwise hashing.
*
* If sd is a nested struct, and if it's nested in a class, the calculated
* hash value will also contain the result of parent class's toHash().
*/
const(char)* code =
"size_t h = 0;" ~
"foreach (i, T; typeof(p.tupleof))" ~
" h += typeid(T).getHash(cast(const void*)&p.tupleof[i]);" ~
"return h;";
fop.fbody = new CompileStatement(loc, new StringExp(loc, cast(char*)code));
Scope* sc2 = sc.push();
sc2.stc = 0;
Expand Down
7 changes: 6 additions & 1 deletion src/opover.d
Expand Up @@ -1238,10 +1238,15 @@ extern (C++) Expression op_overload(Expression e, Scope* sc)
return;
}

/* Rewrite:
/* Do memberwise equality.
* Rewrite:
* e1 == e2
* as:
* e1.tupleof == e2.tupleof
*
* If sd is a nested struct, and if it's nested in a class, it will
* also compare the parent class's equality. Otherwise, compares
* the identity of parent context through void*.
*/
if (e.att1 && t1 == e.att1) return;
if (e.att2 && t2 == e.att2) return;
Expand Down
156 changes: 155 additions & 1 deletion test/runnable/nested.d
Expand Up @@ -2487,6 +2487,159 @@ void test14846()
}

/*******************************************/
// 15422

class App15422(T)
{
this() {}

auto test1(T val)
in {} body // necessary to reproduce the crash
{
struct Foo
{
this(int k) {}
T a;
}

Foo foo;
foo.a = val;

// Frame of test2 function, allocated on heap.
assert(foo.tupleof[$-1] !is null);

//printf("&foo = %p\n", &foo); // stack
//printf("&this = %p\n", &this); // stack?
//printf("foo.vthis = %p\n", foo.tupleof[$-1]); // stack...!?
//assert(cast(void*)&this !is *cast(void**)&foo.tupleof[$-1], "bad");
// BUG: currently foo.vthis set to the address of 'this' variable on the stack.
// It's should be stomped to null, because Foo.vthis is never be used.

int[Foo] map;
map[foo] = 1; // OK <- crash

return foo;
}

auto test2(T val)
//in {} body
{
int closVar;
struct Foo
{
this(int k) { closVar = k; }
// Make val a closure variable.

T a;
}

Foo foo;
foo.a = val;

// Frame of test2 function, allocated on heap.
assert(foo.tupleof[$-1] !is null);

return foo;
}
}

void test15422a()
{
alias App = App15422!int;
App app1 = new App;
{
auto x = app1.test1(1);
auto y = app1.test1(1);
static assert(is(typeof(x) == typeof(y)));

// int (bitwise comparison)
assert(x.a == y.a);

assert(*cast(void**)&x.tupleof[$-1] is *cast(void**)&y.tupleof[$-1]);

// bitwise equality (needOpEquals() and needToHash() returns false)
assert(x == y);

// BUG
//assert(*cast(void**)&x.tupleof[$-1] is null);
//assert(*cast(void**)&y.tupleof[$-1] is null);
auto getZ() { auto z = app1.test1(1); return z; }
auto z = getZ();
assert(x.a == z.a);
//assert(x.tupleof[$-1] is z.tupleof[$-1]); // should pass
//assert(x == z); // should pass

x = y; // OK, x.tupleof[$-1] = y.tupleof[$-1] is a blit copy.
}
App app2 = new App;
{
auto x = app1.test2(1);
auto y = app2.test2(1);
static assert(is(typeof(x) == typeof(y)));

// int (bitwise comparison)
assert(x.a == y.a);

// closure envirionments
assert(*cast(void**)&x.tupleof[$-1] !is *cast(void**)&y.tupleof[$-1]);

// Changed to bitwise equality (needOpEquals() and needToHash() returns false)
assert(x != y); // OK <- crash

x = y; // OK, x.tupleof[$-1] = y.tupleof[$-1] is a blit copy.
}
}

void test15422b()
{
alias App = App15422!string;
App app1 = new App;
{
auto x = app1.test1("a".idup);
auto y = app1.test1("a".idup);
static assert(is(typeof(x) == typeof(y)));

// string (element-wise comparison)
assert(x.a == y.a);

assert(*cast(void**)&x.tupleof[$-1] is *cast(void**)&y.tupleof[$-1]);

// memberwise equality (needToHash() returns true)
assert(x == y);
// Lowered to: x.a == y.a && x.tupleof[$-1] is y.tupleof[$-1]

// BUG
//assert(*cast(void**)&x.tupleof[$-1] is null);
//assert(*cast(void**)&y.tupleof[$-1] is null);
auto getZ() { auto z = app1.test1("a".idup); return z; }
auto z = getZ();
assert(x.a == z.a);
//assert(x.tupleof[$-1] is z.tupleof[$-1]); // should pass
//assert(x == z); // should pass

x = y; // OK, x.tupleof[$-1] = y.tupleof[$-1] is a blit copy.
}
App app2 = new App;
{
auto x = app1.test2("a".idup);
auto y = app2.test2("a".idup);
static assert(is(typeof(x) == typeof(y)));

// string (element-wise comparison)
assert(x.a == y.a);

// closure envirionments
assert(*cast(void**)&x.tupleof[$-1] !is *cast(void**)&y.tupleof[$-1]);

// Changed to memberwise equality (needToHash() returns true)
// Lowered to: x.a == y.a && x.tupleof[$-1] is y.tupleof[$-1]
assert(x != y); // OK <- crash

x = y; // OK, x.tupleof[$-1] = y.tupleof[$-1] is a blit copy.
}
}

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

int main()
{
Expand Down Expand Up @@ -2578,8 +2731,9 @@ int main()
test13861();
test14398();
test14846();
test15422a();
test15422b();

printf("Success\n");
return 0;
}

7 changes: 0 additions & 7 deletions test/runnable/test11.d
Expand Up @@ -1275,9 +1275,6 @@ void test8809()
char test3Bx() { return (new class Object { char bar() { return B.foo(); } }).bar(); }
char test3Cx() { return (new class Object { char bar() { return C.foo(); } }).bar(); }
char test3Dx() { return (new class Object { char bar() { return foo(); } }).bar(); }
char test3By() { return (new class Object { char bar() { return this.outer.B.foo(); } }).bar(); }
char test3Cy() { return (new class Object { char bar() { return this.outer.C.foo(); } }).bar(); }
char test3Dy() { return (new class Object { char bar() { return this.outer. foo(); } }).bar(); }

override char foo() { return 'C'; }
}
Expand Down Expand Up @@ -1311,10 +1308,6 @@ void test8809()
assert(c.test3Bx() == 'B'); // NG('D') -> OK
assert(c.test3Cx() == 'C'); // NG('D') -> OK
assert(c.test3Dx() == 'D');

assert(c.test3By() == 'B');
assert(c.test3Cy() == 'C');
assert(c.test3Dy() == 'D');
}

/**************************************/
Expand Down

0 comments on commit d789012

Please sign in to comment.