code review 7986043: cmd/gc: instrument logical && and ||.

cmd/gc: instrument logical && and ||.

The right operand of a && and || is only executed conditionnally,
so the instrumentation must be more careful. In particular
it should not turn nodes assumed to be cheap after walk into
expensive ones.

Update issue 4228

[remyoudompheng, 2013-03-23 08:36:14]

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

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

[dvyukov, 2013-03-24 12:49:08]

LGTM
Thanks!
Do you see any new races with this? When I've experimented with && ||
instrumentation, some tests crashed but I've found a new race in net package.

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

https://codereview.appspot.com/7986043/diff/5001/src/cmd/gc/racewalk.c#newcod...
src/cmd/gc/racewalk.c:584: // but appendsd rather than prepends.
s/appendsd/appends/

https://codereview.appspot.com/7986043/diff/5001/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (right):

https://codereview.appspot.com/7986043/diff/5001/src/cmd/gc/subr.c#newcode1764
src/cmd/gc/subr.c:1764: // hard with racedetector
space between race and detector please

[remyoudompheng, 2013-03-24 13:12:53]

2013/3/24  <dvyukov@google.com>:
> LGTM
> Thanks!
> Do you see any new races with this? When I've experimented with && ||
> instrumentation, some tests crashed but I've found a new race in net
> package.

I haven't found new races in the standard library. During my
experiments I had issues with sync/atomic and nil addresses (see CL
7981043).

I had a strange lockup in net/http benchmark when running race.bash,
I'll have a closer look first.

Rémy.

[remyoudompheng, 2013-03-25 00:24:15]

On 2013/03/24 13:12:53, remyoudompheng wrote:
> I had a strange lockup in net/http benchmark when running race.bash,
> I'll have a closer look first.

I can reproduce the lockup without the patch so I guess it's not a regression
actually.
I have opened issue 5121 for that.

Rémy.

[remyoudompheng, 2013-03-25 07:39:02]

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

Please take another look.

[remyoudompheng, 2013-03-25 07:42:06]

After fixing the benchmark net/http seems no longer flaky.

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

https://codereview.appspot.com/7986043/diff/5001/src/cmd/gc/racewalk.c#newcod...
src/cmd/gc/racewalk.c:584: // but appendsd rather than prepends.
On 2013/03/24 12:49:08, dvyukov wrote:
> s/appendsd/appends/

Done.

https://codereview.appspot.com/7986043/diff/5001/src/cmd/gc/subr.c
File src/cmd/gc/subr.c (right):

https://codereview.appspot.com/7986043/diff/5001/src/cmd/gc/subr.c#newcode1764
src/cmd/gc/subr.c:1764: // hard with racedetector
On 2013/03/24 12:49:08, dvyukov wrote:
> space between race and detector please 

Done.

[dvyukov, 2013-03-25 13:42:28]

LGTM

[remyoudompheng, 2013-03-25 21:13:00]

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

cmd/gc: instrument logical && and ||.

The right operand of a && and || is only executed conditionnally,
so the instrumentation must be more careful. In particular
it should not turn nodes assumed to be cheap after walk into
expensive ones.

Update issue 4228

R=dvyukov, golang-dev
CC=golang-dev
https://codereview.appspot.com/7986043