code review 6452046: runtime: round return value address in runtime.equal

runtime: round return value address in runtime.equal
     Fixes issue 3866.

[minux1, 2012-07-26 04:27:47]

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

I'd like you to review this change to
https://code.google.com/p/go/

[nigeltao, 2012-07-26 04:40:58]

Should the test file be called arrayequal.go instead of equal.go??

http://codereview.appspot.com/6452046/diff/1002/test/equal.go
File test/equal.go (right):

http://codereview.appspot.com/6452046/diff/1002/test/equal.go#newcode22
test/equal.go:22: fmt.Fprintf(bout, "package main\n")
Would `` strings make some of this easier?

http://codereview.appspot.com/6452046/diff/1002/test/equal.go#newcode35
test/equal.go:35: fmt.Fprintf(bout, "\treturn v == [%d]uint8{2}\n}\n", i)
Can we test for false negatives as well as false positives?

[minux1, 2012-07-26 05:29:13]

On 2012/07/26 04:40:58, nigeltao wrote:
> Should the test file be called arrayequal.go instead of equal.go??
i moved the test file to fixedbugs/bug447.go in Patch Set 5. 
in fact, the bug is in runtime.equal, and i suspect "struct {uint8}" could also
fail, so equal.go is a better name (i only test the array case, because the
length of arguments is what matters)

> http://codereview.appspot.com/6452046/diff/1002/test/equal.go#newcode22
> test/equal.go:22: fmt.Fprintf(bout, "package main\n")
> Would `` strings make some of this easier?
probable, i thought backquote make the left margin ragged if used inline.
anyway, i changed to use backquote string.

> http://codereview.appspot.com/6452046/diff/1002/test/equal.go#newcode35
> test/equal.go:35: fmt.Fprintf(bout, "\treturn v == [%d]uint8{2}\n}\n", i)
> Can we test for false negatives as well as false positives?
sure. i should have tested false negatives, because it will catch more
situations in theory.

[nigeltao, 2012-07-26 06:00:37]

For backquote strings, I was imagining something more like the program below.
WDYT?


package main

import (
    "bytes"
    "fmt"
    "os"
    "strconv"
    "strings"
)

const ntest = 1024

func main() {
    var decls, calls bytes.Buffer

    for i := 1; i <= ntest; i++ {
        s := strconv.Itoa(i)
        decls.WriteString(strings.Replace(decl, "$", s, -1))
        calls.WriteString(strings.Replace(call, "$", s, -1))
    }

    program = strings.Replace(program, "$DECLS", decls.String(), 1)
    program = strings.Replace(program, "$CALLS", calls.String(), 1)
    fmt.Fprint(os.Stdout, program)
}

var program = `package main

var count int

func call(f func() bool) {
    if f() {
        count++
    }
}

$DECLS

func main() {
    $CALLS
    if count != 0 {
        println("failed", count, "case(s)")
    }
}
`

const decl = `
type T$ [$]uint8

func test$() bool {
    v := T${1}
    return v != [$]uint8{1} || v == [$]uint8{2}
}
`

const call = `
    call(test$)
`

[minux1, 2012-07-26 06:20:00]

On 2012/07/26 06:00:37, nigeltao wrote:
> For backquote strings, I was imagining something more like the program below.
> WDYT?
This is much better, thank you very much!

[nigeltao, 2012-07-26 07:39:58]

LGTM, but give rsc an opportunity to look at this before you submit.

http://codereview.appspot.com/6452046/diff/17002/src/pkg/runtime/alg.c
File src/pkg/runtime/alg.c (right):

http://codereview.appspot.com/6452046/diff/17002/src/pkg/runtime/alg.c#newcod...
src/pkg/runtime/alg.c:472: uintptr ret;
Would it be better to keep ret as a bool*, so that pointer arithmetic is always
byte-sized? You can still ROUND a bool*, right?

{
        byte *x, *y;
        bool *ret;
        
        x = (byte*)(&t+1);
        y = x + t->size;
        ret = (bool*)(y + t->size);
        ret = ROUND(ret, sizeof(uintptr));
        t->alg->equal(ret, t->size, x, y);
}

http://codereview.appspot.com/6452046/diff/17002/src/pkg/runtime/alg.c#newcod...
src/pkg/runtime/alg.c:476: ret = (uintptr)y + t->size;
Even if you keep ret as a uintptr instead of bool*, I'd add more parens.
(uintptr)(y + t->size)

[r, 2012-07-26 15:53:10]

http://codereview.appspot.com/6452046/diff/17003/test/fixedbugs/bug447.go
File test/fixedbugs/bug447.go (right):

http://codereview.appspot.com/6452046/diff/17003/test/fixedbugs/bug447.go#new...
test/fixedbugs/bug447.go:7: package main
please put a comment here that explains what this program tests.

[minux1, 2012-07-26 18:18:36]

I found a new problem: this test is not future-proof.

this test depends on gc generating call to runtime.equal for comparing
instance of
T and T's underlying type, but this need not to be the case in the future.

Do I need to find a new way to test this?

[minux1, 2012-07-26 19:20:12]

http://codereview.appspot.com/6452046/diff/17002/src/pkg/runtime/alg.c
File src/pkg/runtime/alg.c (right):

http://codereview.appspot.com/6452046/diff/17002/src/pkg/runtime/alg.c#newcod...
src/pkg/runtime/alg.c:472: uintptr ret;
On 2012/07/26 07:39:58, nigeltao wrote:
> byte-sized? You can still ROUND a bool*, right?
ROUND(x, y) involves x & ~(y - 1), so we can't ROUND a bool*.

[nigeltao, 2012-07-26 23:22:52]

On 27 July 2012 04:18, minux <minux.ma@gmail.com> wrote:
> I found a new problem: this test is not future-proof.
>
> this test depends on gc generating call to runtime.equal for comparing
> instance of
> T and T's underlying type, but this need not to be the case in the future.
>
> Do I need to find a new way to test this?

My opinion is that you should test the language, not the
implementation. So if a future version of gc doesn't use
runtime·equal, this test is still valid, and should still pass.

[minux1, 2012-07-26 23:30:56]

On Thu, Jul 26, 2012 at 4:22 PM, Nigel Tao <nigeltao@golang.org> wrote:

> My opinion is that you should test the language, not the
> implementation. So if a future version of gc doesn't use
> runtime·equal, this test is still valid, and should still pass.
>
so i suppose i should enhance the test to cover more equality test case?

[nigeltao, 2012-07-26 23:34:35]

On 27 July 2012 09:30, minux <minux.ma@gmail.com> wrote:
> so i suppose i should enhance the test to cover more equality test case?

I think what you have now is fine. It tests that a particular bug is fixed.

A hypothetical future change may make this test no longer test a
particular code path in the compiler and runtime, but it is still a
correct program with respect to the language.

[minux1, 2012-07-27 00:11:20]

On 2012/07/26 15:53:10, r wrote:
http://codereview.appspot.com/6452046/diff/17003/test/fixedbugs/bug447.go#new...
> test/fixedbugs/bug447.go:7: package main
> please put a comment here that explains what this program tests.
Done.

[r, 2012-07-27 00:16:28]

http://codereview.appspot.com/6452046/diff/7/test/fixedbugs/bug447.go
File test/fixedbugs/bug447.go (right):

http://codereview.appspot.com/6452046/diff/7/test/fixedbugs/bug447.go#newcode9
test/fixedbugs/bug447.go:9: // return values into account; so in certain cases,
gc generated
s/,//
s/;/,/
s/gc generated/gc-generated/

http://codereview.appspot.com/6452046/diff/7/test/fixedbugs/bug447.go#newcode10
test/fixedbugs/bug447.go:10: // code will read a random bool from stack as the
result of the
s/stack/the stack/

http://codereview.appspot.com/6452046/diff/7/test/fixedbugs/bug447.go#newcode15
test/fixedbugs/bug447.go:15: // underlying []byte will make gc emit call to
runtime.equal,
s/call/calls/

http://codereview.appspot.com/6452046/diff/7/test/fixedbugs/bug447.go#newcode17
test/fixedbugs/bug447.go:17: // be correct (in a sense that it no longer test
runtime.equal).
s/a/the/
s/test/tests/

[minux1, 2012-07-27 00:44:25]

thank you very much for helping me improving my writing!

On 2012/07/27 00:16:28, r wrote:
> http://codereview.appspot.com/6452046/diff/7/test/fixedbugs/bug447.go
> File test/fixedbugs/bug447.go (right):
all done.

[rsc, 2012-07-29 23:45:43]

LGTM

Thanks very much.

[minux1, 2012-07-30 22:32:19]

i'm wondering if we need to round y according to t->align also.
(see patch set 14)

i think for our current usage of runtime.align, y doesn't need
to be aligned by t->align, but adding alignment there would
make us safer if we extend usage of runtime.equal in the future.
what do you think?

[nigeltao, 2012-07-31 05:27:10]

On 31 July 2012 08:32,  <minux.ma@gmail.com> wrote:
> i'm wondering if we need to round y according to t->align also.
> (see patch set 14)

LGTM.

[minux1, 2012-08-01 03:02:52]

*** Submitted as http://code.google.com/p/go/source/detail?r=a7752d169d2e ***

runtime: round return value address in runtime.equal
     Fixes issue 3866.

R=rsc, r, nigeltao
CC=golang-dev
http://codereview.appspot.com/6452046

[rsc, 2012-08-01 21:26:27]

http://codereview.appspot.com/6452046/diff/17009/src/pkg/runtime/alg.c
File src/pkg/runtime/alg.c (right):

http://codereview.appspot.com/6452046/diff/17009/src/pkg/runtime/alg.c#newcod...
src/pkg/runtime/alg.c:475: y = x + ROUND(t->size, t->align);
This is fine but also unnecessary. t->size must necessarily be a multiple of
t->align. Otherwise the second element in an array of two elements would be
misaligned.

[dave_cheney.net, 2012-08-06 01:56:47]

Minux: will you address this final comment by rsc ?

On Thu, Aug 2, 2012 at 7:26 AM,  <rsc@golang.org> wrote:
>
> http://codereview.appspot.com/6452046/diff/17009/src/pkg/runtime/alg.c
> File src/pkg/runtime/alg.c (right):
>
>
http://codereview.appspot.com/6452046/diff/17009/src/pkg/runtime/alg.c#newcod...
> src/pkg/runtime/alg.c:475: y = x + ROUND(t->size, t->align);
> This is fine but also unnecessary. t->size must necessarily be a
> multiple of t->align. Otherwise the second element in an array of two
> elements would be misaligned.
>
> http://codereview.appspot.com/6452046/

[minux1, 2012-08-06 02:05:05]

On 2012/08/01 21:26:27, rsc wrote:
http://codereview.appspot.com/6452046/diff/17009/src/pkg/runtime/alg.c#newcod...
> src/pkg/runtime/alg.c:475: y = x + ROUND(t->size, t->align);
> This is fine but also unnecessary. t->size must necessarily be a multiple of
> t->align. Otherwise the second element in an array of two elements would be
> misaligned.
what i am think is this:
runtime.align is not designed for comparing array only, is it?
if we ever pass non-array type T to it, then the extra round is necessary,
right?

[rsc, 2012-08-06 19:48:09]

> what i am think is this:
> runtime.align is not designed for comparing array only, is it?
> if we ever pass non-array type T to it, then the extra round is
> necessary, right?

no. a type T's size is always a multiple of its alignment.
the array was only an example of why this must be true (so that every
element of [10]T is properly aligned). arrays don't have to be
involved for it to be true. it's always true.

russ

[minux1, 2012-08-07 01:41:38]

On 2012/08/06 19:48:09, rsc wrote:
> no. a type T's size is always a multiple of its alignment.
> the array was only an example of why this must be true (so that every
> element of [10]T is properly aligned). arrays don't have to be
> involved for it to be true. it's always true.
ok, i was confused by some ROUND code in hashmap.c,
and i understand now. will remove this unneeded ROUND.