FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
cmd/gc: fix handling of struct padding in hash/eq.

The test case of issue 4585 was not passing due to
miscalculation of memequal args, and the previous fix
does not handle padding at the end of a struct.

Handling of padding at end of structs also fixes the case
of [n]T where T is such a padded struct.

Fixes issue 4585.
(again)

Please review this at https://codereview.appspot.com/7133059/

Affected files:
M src/cmd/gc/subr.c
M test/fixedbugs/issue4585.go


Index: src/cmd/gc/subr.c
===================================================================
--- a/src/cmd/gc/subr.c
+++ b/src/cmd/gc/subr.c
@@ -504,12 +504,17 @@
return n;
}

+// ispaddedfield returns whether the given field
+// is followed by padding. For the case where t is
+// the last field, total gives the size of the enclosing struct.
static int
-ispaddedfield(Type *t)
+ispaddedfield(Type *t, vlong total)
{
if(t->etype != TFIELD)
fatal("ispaddedfield called non-field %T", t);
- return t->down != T && t->width + t->type->width != t->down->width;
+ if(t->down == T)
+ return t->width + t->type->width != total;
+ return t->width + t->type->width != t->down->width;
}

int
@@ -591,7 +596,7 @@
for(t1=t->type; t1!=T; t1=t1->down) {
// Blank fields and padding must be ignored,
// so need special compare.
- if(isblanksym(t1->sym) || ispaddedfield(t1)) {
+ if(isblanksym(t1->sym) || ispaddedfield(t1, t->width)) {
ret = -1;
continue;
}
@@ -2619,7 +2624,7 @@
Node *hashel;
Type *first, *t1;
int old_safemode;
- int64 size, mul;
+ int64 size, mul, offend;

if(debug['r'])
print("genhash %S %T\n", sym, t);
@@ -2705,24 +2710,21 @@
// Walk the struct using memhash for runs of AMEM
// and calling specific hash functions for the others.
first = T;
+ offend = 0;
for(t1=t->type;; t1=t1->down) {
if(t1 != T && algtype1(t1->type, nil) == AMEM && !isblanksym(t1->sym)) {
+ offend = t1->width + t1->type->width;
if(first == T)
first = t1;
// If it's a memory field but it's padded, stop here.
- if(ispaddedfield(t1))
+ if(ispaddedfield(t1, t->width))
t1 = t1->down;
else
continue;
}
// Run memhash for fields up to this one.
if(first != T) {
- if(first->down == t1)
- size = first->type->width;
- else if(t1 == T)
- size = t->width - first->width; // first->width is offset
- else
- size = t1->width - first->width; // both are offsets
+ size = offend - first->width; // first->width is offset
hashel = hashmem(first->type);
// hashel(h, size, &p.first)
call = nod(OCALL, hashel, N);
@@ -2856,6 +2858,7 @@
Type *t1, *first;
int old_safemode;
int64 size;
+ int64 offend;

if(debug['r'])
print("geneq %S %T\n", sym, t);
@@ -2929,12 +2932,14 @@
// and calling specific equality tests for the others.
// Skip blank-named fields.
first = T;
+ offend = 0;
for(t1=t->type;; t1=t1->down) {
if(t1 != T && algtype1(t1->type, nil) == AMEM && !isblanksym(t1->sym)) {
+ offend = t1->width + t1->type->width;
if(first == T)
first = t1;
// If it's a memory field but it's padded, stop here.
- if(ispaddedfield(t1))
+ if(ispaddedfield(t1, t->width))
t1 = t1->down;
else
continue;
@@ -2952,10 +2957,7 @@
fn->nbody = list(fn->nbody, eqfield(np, nq, newname(first->sym),
neq));
} else {
// More than two fields: use memequal.
- if(t1 == T)
- size = t->width - first->width; // first->width is offset
- else
- size = t1->width - first->width; // both are offsets
+ size = offend - first->width; // first->width is offset
fn->nbody = list(fn->nbody, eqmem(np, nq, newname(first->sym), size,
neq));
}
first = T;
Index: test/fixedbugs/issue4585.go
===================================================================
--- a/test/fixedbugs/issue4585.go
+++ b/test/fixedbugs/issue4585.go
@@ -32,6 +32,20 @@
A, _, B int32
}

+// V has padding but not on the first field.
+type V struct {
+ A1, A2, A3 int32
+ B int16
+ C int32
+}
+
+// W has padding at the end.
+type W struct {
+ A1, A2, A3 int32
+ B int32
+ C int8
+}
+
func test1() {
var a, b U
m := make(map[U]int)
@@ -84,8 +98,54 @@
}
}

+func test4() {
+ var a, b V
+ m := make(map[V]int)
+
+ copy((*[20]byte)(unsafe.Pointer(&a))[:], "Hello World, Gopher!")
+ a.A1, a.A2, a.A3, a.B, a.C = 1, 2, 3, 4, 5
+ b.A1, b.A2, b.A3, b.B, b.C = 1, 2, 3, 4, 5
+
+ if a != b {
+ panic("broken equality: a != b")
+ }
+
+ m[a] = 1
+ m[b] = 2
+ if len(m) == 2 {
+ panic("broken hash: len(m) == 2")
+ }
+ if m[a] != 2 {
+ panic("m[a] != 2")
+ }
+}
+
+func test5() {
+ var a, b W
+ m := make(map[W]int)
+
+ copy((*[20]byte)(unsafe.Pointer(&a))[:], "Hello World, Gopher!")
+ a.A1, a.A2, a.A3, a.B, a.C = 1, 2, 3, 4, 5
+ b.A1, b.A2, b.A3, b.B, b.C = 1, 2, 3, 4, 5
+
+ if a != b {
+ panic("broken equality: a != b")
+ }
+
+ m[a] = 1
+ m[b] = 2
+ if len(m) == 2 {
+ panic("broken hash: len(m) == 2")
+ }
+ if m[a] != 2 {
+ panic("m[a] != 2")
+ }
+}
+
func main() {
test1()
test2()
test3()
+ test4()
+ test5()
}

Search Discussions

  • Rsc at Jan 18, 2013 at 7:10 pm
  • Remyoudompheng at Jan 18, 2013 at 9:42 pm
    *** Submitted as
    https://code.google.com/p/go/source/detail?r=b555da941249 ***

    cmd/gc: fix handling of struct padding in hash/eq.

    The test case of issue 4585 was not passing due to
    miscalculation of memequal args, and the previous fix
    does not handle padding at the end of a struct.

    Handling of padding at end of structs also fixes the case
    of [n]T where T is such a padded struct.

    Fixes issue 4585.
    (again)

    R=golang-dev, rsc
    CC=golang-dev
    https://codereview.appspot.com/7133059


    https://codereview.appspot.com/7133059/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedJan 18, '13 at 6:14p
activeJan 18, '13 at 9:42p
posts3
users2
websitegolang.org

2 users in discussion

Remyoudompheng: 2 posts Rsc: 1 post

People

Translate

site design / logo © 2022 Grokbase