code review 76520045: runtime/race: add test for issue 7561.

runtime/race: add test for issue 7561.

[remyoudompheng, 2014-03-20 05:55:59]

Hello rsc@golang.org, iant@golang.org, khr@golang.org, dvyukov@google.com (cc:
golang-codereviews@googlegroups.com),

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

[dvyukov, 2014-03-20 06:13:33]

LGTM, but wait for somebody else
I see how the compiler change fixes the issue, but I don't know whether it's the
right way to approach it

[rsc, 2014-03-24 18:57:06]

https://codereview.appspot.com/76520045/diff/40001/src/cmd/gc/racewalk.c
File src/cmd/gc/racewalk.c (right):

https://codereview.appspot.com/76520045/diff/40001/src/cmd/gc/racewalk.c#newc...
src/cmd/gc/racewalk.c:180: // Blocks are used for multiple return function
calls.
The real problem is this assumption. An OBLOCK node is used to wrap a sequences
of statements so that it can appear as a single Node*, like the GNU C ({ })
syntax. It is a detail of the internal representation, nothing more. In
particular, there is no special case for function calls with multiple returns.
There is no guarantee that if the block contains an
OCALLFUNC/OCALLMETH/OCALLINTER it will be at the top of the block, nor is there
a guarantee that there isn't other work in the block too. This comment is
ascribing intent and meaning to what is at best an accident of the current
implementation. Perhaps this is true most of the time today, but if so that is
news to me, and I see no reason to believe it will continue to be true. The
functions list1 and list are pretty aggressive about removing OBLOCK nodes when
possible, to linearize larger lists.

If it is important to treat the call and its subsequent assignments as an atomic
unit, then a more robust thing to do is to iterate over the block's list looking
for that pattern and handling it anywhere in the block.

Something like:

for(l=n->list; l != nil; ) {
    switch(l->n->op) {
    case OCALLFUNC:
    case OCALLMETH:
    case OCALLINTER:
        handle, and advance l to next thing in block we want to annotate,
        skipping over whatever needs skipping
    default:
        instr = nil;
        racewalknode(&l->n, &inst, 0, 0);
        l->n->ninit = concat(l->n->ninit, instr);
        l = l->next;
        break;
    }
}

[rsc, 2014-04-03 17:33:58]

My recent changes may have made this unnecessary, although I still believe the
code should be revised as I suggested.

[rsc, 2014-04-17 03:08:53]

R=close

The test case in the issue works for me at tip, so it does look like I fixed it
(not trying to). I still think the code should be revised, but not for 1.3.

[remyoudompheng, 2014-04-19 10:37:31]

Hello rsc@golang.org, iant@golang.org, khr@golang.org, dvyukov@google.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[dvyukov, 2014-04-21 12:28:26]

LGTM

[remyoudompheng, 2014-04-21 15:21:14]

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

runtime/race: add test for issue 7561.

LGTM=dvyukov
R=rsc, iant, khr, dvyukov
CC=golang-codereviews
https://codereview.appspot.com/76520045