Issue 9910043: code review 9910043: cmd/gc: avoid creating circular lists when compiling wi... (Closed)
Descriptioncmd/gc: avoid creating circular lists when compiling with race detector.
Fixes issue 5431.
Patch Set 1 #Patch Set 2 : diff -r fd791d51e476 https://code.google.com/p/go/ #Patch Set 3 : diff -r fd791d51e476 https://code.google.com/p/go/ #Patch Set 4 : diff -r fd791d51e476 https://code.google.com/p/go/ #
Total comments: 3
Patch Set 5 : diff -r bc2993c5834f https://code.google.com/p/go/ #Patch Set 6 : diff -r bc2993c5834f https://code.google.com/p/go/ #Patch Set 7 : diff -r a14bbc9436a3 https://code.google.com/p/go/ #MessagesTotal messages: 23
Hello golang-dev@googlegroups.com (cc: dvyukov@google.com), I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
I got a test for this, but I'm not sure where to put race detector compilation tests.
Sign in to reply to this message.
On 2013/05/31 19:03:38, DMorsing wrote: > I got a test for this, but I'm not sure where to put race detector compilation > tests. Whoops, put down the wrong issue. Correct issue is 5431.
Sign in to reply to this message.
R=dvyukov (assigned by r)
Sign in to reply to this message.
On 2013/5/31 <daniel.morsing@gmail.com> wrote: > I got a test for this, but I'm not sure where to put race detector > compilation tests. > > https://codereview.appspot.com/9910043/ I think it's ok to put them in src/pkg/runtime/race/mop_test.go with the other &&/|| test.
Sign in to reply to this message.
PTAL.
Sign in to reply to this message.
On 2013/05/31 20:27:08, remyoudompheng wrote: > On 2013/5/31 <mailto:daniel.morsing@gmail.com> wrote: > > I got a test for this, but I'm not sure where to put race detector > > compilation tests. > > > > https://codereview.appspot.com/9910043/ > > I think it's ok to put them in src/pkg/runtime/race/mop_test.go with > the other &&/|| test. I think src/pkg/runtime/race/testdata/regression_test.go is a better place. The file contains similar snippets of code that caused problems in the past. mop_test.go contains real race/no race Test's.
Sign in to reply to this message.
https://codereview.appspot.com/9910043/diff/13001/src/cmd/gc/racewalk.c File src/cmd/gc/racewalk.c (right): https://codereview.appspot.com/9910043/diff/13001/src/cmd/gc/racewalk.c#newco... src/cmd/gc/racewalk.c:261: // if right has an non-empty init list, l will be pointed at it. how does it happen? as far as I see, if n->right is e.g. OIND, then l will contain just the new instrumentation node
Sign in to reply to this message.
https://codereview.appspot.com/9910043/diff/13001/src/cmd/gc/racewalk.c File src/cmd/gc/racewalk.c (right): https://codereview.appspot.com/9910043/diff/13001/src/cmd/gc/racewalk.c#newco... src/cmd/gc/racewalk.c:261: // if right has an non-empty init list, l will be pointed at it. On 2013/06/03 06:57:08, dvyukov wrote: > how does it happen? > as far as I see, if n->right is e.g. OIND, then l will contain just the new > instrumentation node Non-empty init lists only happen when inlining. Inlining will put init lists on expressions which call functions that are inlined. The racewalknode above will eventually call mkcall which will call walkexpr on its argument. walkexpr will append the init list of the expression onto the init argument given. We end up with l being the same list at n->right. Another fix is to make treecopy copy init lists, but this means that we're executing the same init statements twice and since inlining create gotos, you can't copy this list without ending up with duplicate goto labels.
Sign in to reply to this message.
On 2013/06/03 08:20:31, DMorsing wrote: > https://codereview.appspot.com/9910043/diff/13001/src/cmd/gc/racewalk.c > File src/cmd/gc/racewalk.c (right): > > https://codereview.appspot.com/9910043/diff/13001/src/cmd/gc/racewalk.c#newco... > src/cmd/gc/racewalk.c:261: // if right has an non-empty init list, l will be > pointed at it. > On 2013/06/03 06:57:08, dvyukov wrote: > > how does it happen? > > as far as I see, if n->right is e.g. OIND, then l will contain just the new > > instrumentation node > > Non-empty init lists only happen when inlining. Inlining will put init lists on > expressions which call functions that are inlined. > > The racewalknode above will eventually call mkcall which will call walkexpr on > its argument. walkexpr will append the init list of the expression onto the init > argument given. We end up with l being the same list at n->right. > > Another fix is to make treecopy copy init lists, but this means that we're > executing the same init statements twice and since inlining create gotos, you > can't copy this list without ending up with duplicate goto labels. So your fix works when init == l. Is it possible that: * init is non empty * l is appended to init * l is appended to init *again*. Then your check would not be enough, do you see a scenario where it would happen?
Sign in to reply to this message.
On 2013/06/03 08:23:54, remyoudompheng wrote: > On 2013/06/03 08:20:31, DMorsing wrote: > > https://codereview.appspot.com/9910043/diff/13001/src/cmd/gc/racewalk.c > > File src/cmd/gc/racewalk.c (right): > > > > > https://codereview.appspot.com/9910043/diff/13001/src/cmd/gc/racewalk.c#newco... > > src/cmd/gc/racewalk.c:261: // if right has an non-empty init list, l will be > > pointed at it. > > On 2013/06/03 06:57:08, dvyukov wrote: > > > how does it happen? > > > as far as I see, if n->right is e.g. OIND, then l will contain just the new > > > instrumentation node > > > > Non-empty init lists only happen when inlining. Inlining will put init lists > on > > expressions which call functions that are inlined. > > > > The racewalknode above will eventually call mkcall which will call walkexpr on > > its argument. walkexpr will append the init list of the expression onto the > init > > argument given. We end up with l being the same list at n->right. > > > > Another fix is to make treecopy copy init lists, but this means that we're > > executing the same init statements twice and since inlining create gotos, you > > can't copy this list without ending up with duplicate goto labels. > > So your fix works when init == l. > Is it possible that: > * init is non empty > * l is appended to init > * l is appended to init *again*. > > Then your check would not be enough, do you see a scenario where it would > happen? You got it backwards, init is appended to l and l is always empty, so the check is always valid.
Sign in to reply to this message.
https://codereview.appspot.com/9910043/diff/13001/src/cmd/gc/racewalk.c File src/cmd/gc/racewalk.c (right): https://codereview.appspot.com/9910043/diff/13001/src/cmd/gc/racewalk.c#newco... src/cmd/gc/racewalk.c:261: // if right has an non-empty init list, l will be pointed at it. On 2013/06/03 08:20:32, DMorsing wrote: > On 2013/06/03 06:57:08, dvyukov wrote: > > how does it happen? > > as far as I see, if n->right is e.g. OIND, then l will contain just the new > > instrumentation node > > Non-empty init lists only happen when inlining. Inlining will put init lists on > expressions which call functions that are inlined. > > The racewalknode above will eventually call mkcall which will call walkexpr on > its argument. walkexpr will append the init list of the expression onto the init > argument given. We end up with l being the same list at n->right. > > Another fix is to make treecopy copy init lists, but this means that we're > executing the same init statements twice and since inlining create gotos, you > can't copy this list without ending up with duplicate goto labels. Should we then remember n->right->ninit in l or in a separate var, then set n->right->ninit to nil, then call racewalknode(&n->right), and then combine n->right->ninit and instrumentation? I am sure I've seen such code somewhere (probably in walk()). However, then we will need to racewalk n->right->ninit separately. Most likely what I am suggesting is wrong...
Sign in to reply to this message.
On 2013/06/03 08:45:45, dvyukov wrote: > Should we then remember n->right->ninit in l or in a separate var, then set > n->right->ninit to nil, then call racewalknode(&n->right), and then combine > n->right->ninit and instrumentation? > I am sure I've seen such code somewhere (probably in walk()). > However, then we will need to racewalk n->right->ninit separately. > Most likely what I am suggesting is wrong... If you move the test into regression_test.go, and you and Remy agree, and the tests pass, then it's fine with me.
Sign in to reply to this message.
PTAL.
Sign in to reply to this message.
This doesn't seem right. The correct behavior seems unlikely to depend on the exact pointer values here. Either n->right needs to be put onto the init list or not. Do we need a new variable to keep the two possible lists separate?
Sign in to reply to this message.
On 2013/06/03 19:50:42, rsc wrote: > This doesn't seem right. The correct behavior seems unlikely to depend on the > exact pointer values here. Either n->right needs to be put onto the init list or > not. Do we need a new variable to keep the two possible lists separate? We still need to instrument the init list, so we can't set it to nil before racewalking the node and the instrumentation has to be inline in the list to have the right ordering, so we can't add a separate list. How about having a self concat check in appendinit?
Sign in to reply to this message.
On 2013/06/06 15:32:27, DMorsing wrote: > On 2013/06/03 19:50:42, rsc wrote: > > This doesn't seem right. The correct behavior seems unlikely to depend on the > > exact pointer values here. Either n->right needs to be put onto the init list > or > > not. Do we need a new variable to keep the two possible lists separate? > > We still need to instrument the init list, so we can't set it to nil before > racewalking the node and the instrumentation has to be inline in the list to > have the right ordering, so we can't add a separate list. > > How about having a self concat check in appendinit? Isn't it the same as what we have now (just with a check in a different place)? Can't we set init to nil, walk node, manually walk saved init list, and then combine it all in proper order?
Sign in to reply to this message.
On 2013/06/06 15:39:10, dvyukov wrote: > On 2013/06/06 15:32:27, DMorsing wrote: > > On 2013/06/03 19:50:42, rsc wrote: > > > This doesn't seem right. The correct behavior seems unlikely to depend on > the > > > exact pointer values here. Either n->right needs to be put onto the init > list > > or > > > not. Do we need a new variable to keep the two possible lists separate? > > > > We still need to instrument the init list, so we can't set it to nil before > > racewalking the node and the instrumentation has to be inline in the list to > > have the right ordering, so we can't add a separate list. > > > > How about having a self concat check in appendinit? > > Isn't it the same as what we have now (just with a check in a different place)? > > Can't we set init to nil, walk node, manually walk saved init list, and then > combine it all in proper order? Oh, I thought the race instrumentation was interleaved with the original code somehow in the init list. Will do.
Sign in to reply to this message.
PTAL.
Sign in to reply to this message.
On 2013/06/06 16:10:31, DMorsing wrote: > PTAL. looks good, but wait for Russ
Sign in to reply to this message.
LGTM Thanks. That logic I believe!
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=c56c0d1f613e *** cmd/gc: avoid creating circular lists when compiling with race detector. Fixes issue 5431. R=dvyukov, remyoudompheng, rsc CC=gobot, golang-dev https://codereview.appspot.com/9910043
Sign in to reply to this message.
|
