FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com,

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


Description:
runtime/race: more precise handling of finalizers
Currently race detector runtime just disables race detection in the
finalizer goroutine.
It has false positives when a finalizer writes to shared memory -- the
race with finalizer is reported in a normal goroutine that accesses the
same memory.
After this change I am going to synchronize the finalizer goroutine with
the rest of the world in racefingo(). This is closer to what happens in
reality and so
does not have false positives.

Please review this at http://codereview.appspot.com/6810095/

Affected files:
M src/pkg/runtime/mgc0.c


Index: src/pkg/runtime/mgc0.c
===================================================================
--- a/src/pkg/runtime/mgc0.c
+++ b/src/pkg/runtime/mgc0.c
@@ -1137,9 +1137,6 @@
byte *frame;
uint32 framesz, framecap, i;

- if(raceenabled)
- runtime·racefingo();
-
frame = nil;
framecap = 0;
for(;;) {
@@ -1156,6 +1153,8 @@
runtime·park(nil, nil, "finalizer wait");
continue;
}
+ if(raceenabled)
+ runtime·racefingo();
for(; fb; fb=next) {
next = fb->next;
for(i=0; i<fb->cnt; i++) {

Search Discussions

  • Dvyukov at Nov 9, 2012 at 11:24 am

    On 2012/11/08 14:27:59, dvyukov wrote:
    Hello mailto:golang-dev@googlegroups.com,
    I'd like you to review this change to
    https://dvyukov%2540google.com%40code.google.com/p/go/
    I've added here necessary runtime changes and the README file.
    The runtime is built on revision 167609.


    https://codereview.appspot.com/6810095/
  • Minux Ma at Nov 9, 2012 at 11:29 am
    please update the description.


    https://codereview.appspot.com/6810095/diff/5002/src/pkg/runtime/race/README
    File src/pkg/runtime/race/README (right):

    https://codereview.appspot.com/6810095/diff/5002/src/pkg/runtime/race/README#newcode10
    src/pkg/runtime/race/README:10: You may need gcc >= 4.6.1. gcc 4.3
    produces bad syso files with weak symbols
    is this still the case? CL 6783050 just modified cmd/ld to support weak
    symbols.
    sorry i don't have gcc 4.3 to test.
    if it's still true, could you please mail me a copy of bad linux/amd64
    syso off
    list so that i can fix the issue in cmd/6l?

    https://codereview.appspot.com/6810095/
  • Dvyukov at Nov 9, 2012 at 11:31 am

    On 2012/11/09 11:29:28, minux wrote:
    please update the description.
    Done.


    https://codereview.appspot.com/6810095/diff/5002/src/pkg/runtime/race/README
    File src/pkg/runtime/race/README (right):

    https://codereview.appspot.com/6810095/diff/5002/src/pkg/runtime/race/README#newcode10
    src/pkg/runtime/race/README:10: You may need gcc >= 4.6.1. gcc 4.3
    produces bad
    syso files with weak symbols
    is this still the case? CL 6783050 just modified cmd/ld to support
    weak symbols.
    sorry i don't have gcc 4.3 to test.
    if it's still true, could you please mail me a copy of bad linux/amd64 syso off
    list so that i can fix the issue in cmd/6l?
    I will try to reproduce it.



    https://codereview.appspot.com/6810095/
  • Dvyukov at Nov 9, 2012 at 11:43 am

    On 2012/11/09 11:31:55, dvyukov wrote:
    On 2012/11/09 11:29:28, minux wrote:
    please update the description.
    Done.
    > >
    https://codereview.appspot.com/6810095/diff/5002/src/pkg/runtime/race/README
    File src/pkg/runtime/race/README (right):
    https://codereview.appspot.com/6810095/diff/5002/src/pkg/runtime/race/README#newcode10
    src/pkg/runtime/race/README:10: You may need gcc >= 4.6.1. gcc 4.3
    produces
    bad
    syso files with weak symbols
    is this still the case? CL 6783050 just modified cmd/ld to support
    weak
    symbols.
    sorry i don't have gcc 4.3 to test.
    if it's still true, could you please mail me a copy of bad
    linux/amd64 syso
    off
    list so that i can fix the issue in cmd/6l?
    I will try to reproduce it.
    Seem to work now.

    However gcc 4.4 still fails when I build in debug mode:

    gotsan.cc:1485: sorry, unimplemented: inlining failed in call to ‘void
    __tsan::MemoryAccessImpl(__tsan::ThreadState*, __sanitizer::uptr, int,
    bool, __tsan::FastState, __sanitizer::u64*, __tsan::Shadow)’: function
    not inlinable

    I rephrased it as "Tested with gcc 4.6.1 and 4.7.0."


    https://codereview.appspot.com/6810095/
  • Minux at Nov 11, 2012 at 5:49 pm

    On Fri, Nov 9, 2012 at 7:43 PM, wrote:
    Seem to work now.
    good to hear.
    However gcc 4.4 still fails when I build in debug mode:

    gotsan.cc:1485: sorry, unimplemented: inlining failed in call to ‘void
    __tsan::MemoryAccessImpl(__**tsan::ThreadState*, __sanitizer::uptr, int,
    bool, __tsan::FastState, __sanitizer::u64*, __tsan::Shadow)’: function
    not inlinable

    I rephrased it as "Tested with gcc 4.6.1 and 4.7.0.“
    the README looks fine to me now.
  • Rsc at Nov 12, 2012 at 8:41 pm
  • Dvyukov at Nov 14, 2012 at 1:23 pm
    *** Submitted as
    http://code.google.com/p/go/source/detail?r=d1854dd425b2 ***

    runtime/race: more precise handling of finalizers
    Currently race detector runtime just disables race detection in the
    finalizer goroutine.
    It has false positives when a finalizer writes to shared memory -- the
    race with finalizer is reported in a normal goroutine that accesses the
    same memory.
    After this change I am going to synchronize the finalizer goroutine with
    the rest of the world in racefingo(). This is closer to what happens in
    reality and so
    does not have false positives.
    And also add README file with instructions how to build the runtime.

    R=golang-dev, minux.ma, rsc
    CC=golang-dev
    http://codereview.appspot.com/6810095


    http://codereview.appspot.com/6810095/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 8, '12 at 2:28p
activeNov 14, '12 at 1:23p
posts8
users3
websitegolang.org

3 users in discussion

Dvyukov: 5 posts Minux: 2 posts Rsc: 1 post

People

Translate

site design / logo © 2022 Grokbase