Add skia_tsan_build to match skia_asan_build and fix one example race.

Add skia_tsan_build to match skia_asan_build and fix one example race.

R=mtklein@google.com

Committed: https://code.google.com/p/skia/source/detail?r=11464

[bungeman-skia, 2013-09-25 20:06:27]

Just to show the kinds of things that tsan is going to have issues with, the
first warning by tsan (fixed by this change) was

WARNING: ThreadSanitizer: data race (pid=18943)
  Read of size 4 at 0x7fff504ad304 by thread T1:
    #0 DebugfReporter::onStart(skiatest::Test*)
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/skia_test.cpp:89
(exe+0x0000003eb507)
    #1 skiatest::Reporter::startTest(skiatest::Test*)
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/Test.cpp:31
(exe+0x000000400394)
    #2 skiatest::Test::run()
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/Test.cpp:103
(exe+0x000000400a4b)
    #3 SkTestRunnable::run()
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/skia_test.cpp:150
(exe+0x0000003eab16)
    #4 SkThreadPool::Loop(void*)
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../src/utils/SkThreadPool.cpp:83
(exe+0x000000b1f317)
    #5 thread_start(void*)
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../src/utils/SkThreadUtils_pthread.cpp:66
(exe+0x000000b5bde0)

  Previous atomic write of size 4 at 0x7fff504ad304 by thread T2:
    #0 __tsan_atomic32_fetch_add
/usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cc:465
(exe+0x00000023ad4d)
    #1 sk_atomic_inc(int*)
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../src/ports/SkThread_pthread.cpp:34
(exe+0x000000afc12a)
    #2 DebugfReporter::onStart(skiatest::Test*)
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/skia_test.cpp:88
(exe+0x0000003eb4c1)
    #3 skiatest::Reporter::startTest(skiatest::Test*)
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/Test.cpp:31
(exe+0x000000400394)
    #4 skiatest::Test::run()
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/Test.cpp:103
(exe+0x000000400a4b)
    #5 SkTestRunnable::run()
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/skia_test.cpp:150
(exe+0x0000003eab16)
    #6 SkThreadPool::Loop(void*)
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../src/utils/SkThreadPool.cpp:83
(exe+0x000000b1f317)
    #7 thread_start(void*)
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../src/utils/SkThreadUtils_pthread.cpp:66
(exe+0x000000b5bde0)

  Location is stack of main thread.

  Thread T1 (tid=18952, running) created by main thread at:
    #0 pthread_create
/usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:820
(exe+0x000000215190)
    #1 SkThread
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../src/utils/SkThreadUtils_pthread.cpp:75
(exe+0x000000b5bbcb)
    #2 SkThreadPool
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../src/utils/SkThreadPool.cpp:35
(exe+0x000000b1ef86)
    #3 tool_main(int, char**)
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/skia_test.cpp:226
(exe+0x0000003e7b99)
    #4 main
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/skia_test.cpp:271
(exe+0x0000003e840d)

  Thread T2 (tid=18953, running) created by main thread at:
    #0 pthread_create
/usr/local/google/work/chromium/src/third_party/llvm/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:820
(exe+0x000000215190)
    #1 SkThread
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../src/utils/SkThreadUtils_pthread.cpp:75
(exe+0x000000b5bbcb)
    #2 SkThreadPool
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../src/utils/SkThreadPool.cpp:35
(exe+0x000000b1ef86)
    #3 tool_main(int, char**)
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/skia_test.cpp:226
(exe+0x0000003e7b99)
    #4 main
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/skia_test.cpp:271
(exe+0x0000003e840d)

SUMMARY: ThreadSanitizer: data race
/usr/local/google/home/bungeman/skia/branches/test/out/Debug/../../tests/skia_test.cpp:89
DebugfReporter::onStart(skiatest::Test*)

[mtklein, 2013-09-25 20:12:17]

lgtm

[borenet, 2013-09-25 20:59:02]

Awesome.  Should we add something analogous to tools/asan_build, which sets
CC/CXX/LINK to use clang?

[bungeman-skia, 2013-09-25 21:07:47]

On 2013/09/25 20:59:02, borenet wrote:
> Awesome.  Should we add something analogous to tools/asan_build, which sets
> CC/CXX/LINK to use clang?

First, some meme text:
I am but a simple engineer, your build scripts confuse and frighten me.
Use 'make'? What am I, a farmer?

I was previously unaware of the existence of tools/asan_build. I'm sure that
someone might possibly find such a thing tweaked to s/_asan_/_tsan_/ useful.

[mtklein, 2013-09-25 21:39:30]

On 2013/09/25 21:07:47, bungeman1 wrote:
> On 2013/09/25 20:59:02, borenet wrote:
> > Awesome.  Should we add something analogous to tools/asan_build, which sets
> > CC/CXX/LINK to use clang?
> 
> First, some meme text:
> I am but a simple engineer, your build scripts confuse and frighten me.
> Use 'make'? What am I, a farmer?
> 
> I was previously unaware of the existence of tools/asan_build. I'm sure that
> someone might possibly find such a thing tweaked to s/_asan_/_tsan_/ useful.

Yeah, personally I think we've probably tipped to the point where we should
probably assume we're using ninja.

tools/asan_build is news to me too.  I took a look at and and I had a thought. 
As a Skia dev, the most useful line of that whole file is 
  export GYP_DEFINES="skia_asan_build=1 ${GYP_DEFINES}"

That is, it reminds me the thing I need to do turn on asan.  Maybe instead of
foo_build files, we can have a build_options.readme file?
  CXX=foo                            // use foo to compile C++ code
  skia_arch_width=[32,64]   // controls compiler -m flag
  skia_clang_build=1           // enable extra warning flags only Clang knows
about
  skia_gpu=0                       // disable GPU support all over the codebase
  skia_asan_build=1            // builds with asan, requires GCC >= 4.8 or Clang
>= 3.2
  skia_tsan_build=1             // builds with tsan, requires ....
etc ?

[borenet, 2013-09-25 21:48:52]

On 2013/09/25 21:39:30, mtklein wrote:
> On 2013/09/25 21:07:47, bungeman1 wrote:
> > On 2013/09/25 20:59:02, borenet wrote:
> > > Awesome.  Should we add something analogous to tools/asan_build, which
sets
> > > CC/CXX/LINK to use clang?
> > 
> > First, some meme text:
> > I am but a simple engineer, your build scripts confuse and frighten me.
> > Use 'make'? What am I, a farmer?
> > 
> > I was previously unaware of the existence of tools/asan_build. I'm sure that
> > someone might possibly find such a thing tweaked to s/_asan_/_tsan_/ useful.
> 
> Yeah, personally I think we've probably tipped to the point where we should
> probably assume we're using ninja.
> 
> tools/asan_build is news to me too.  I took a look at and and I had a thought.

> As a Skia dev, the most useful line of that whole file is 
>   export GYP_DEFINES="skia_asan_build=1 ${GYP_DEFINES}"
> 
> That is, it reminds me the thing I need to do turn on asan.  Maybe instead of
> foo_build files, we can have a build_options.readme file?
>   CXX=foo                            // use foo to compile C++ code
>   skia_arch_width=[32,64]   // controls compiler -m flag
>   skia_clang_build=1           // enable extra warning flags only Clang knows
> about
>   skia_gpu=0                       // disable GPU support all over the
codebase
>   skia_asan_build=1            // builds with asan, requires GCC >= 4.8 or
Clang
> >= 3.2
>   skia_tsan_build=1             // builds with tsan, requires ....
> etc ?

I was under the impression that the vast majority were using make.  If such a
readme would be helpful, then we can certainly add it.  The asan_build script
primarily exists to set those variables, and if they didn't need to be set,
adding the gyp_define would be sufficient.  I propose that, if we're going to
add that readme, it should be auto-generated from gyp/common_variables.gypi.  Or
maybe common_variables just needs better formatting/commenting so that it can
serve as the build readme.

[mtklein, 2013-09-25 21:50:36]

On 2013/09/25 21:48:52, borenet wrote:
> On 2013/09/25 21:39:30, mtklein wrote:
> > On 2013/09/25 21:07:47, bungeman1 wrote:
> > > On 2013/09/25 20:59:02, borenet wrote:
> > > > Awesome.  Should we add something analogous to tools/asan_build, which
> sets
> > > > CC/CXX/LINK to use clang?
> > > 
> > > First, some meme text:
> > > I am but a simple engineer, your build scripts confuse and frighten me.
> > > Use 'make'? What am I, a farmer?
> > > 
> > > I was previously unaware of the existence of tools/asan_build. I'm sure
that
> > > someone might possibly find such a thing tweaked to s/_asan_/_tsan_/
useful.
> > 
> > Yeah, personally I think we've probably tipped to the point where we should
> > probably assume we're using ninja.
> > 
> > tools/asan_build is news to me too.  I took a look at and and I had a
thought.
> 
> > As a Skia dev, the most useful line of that whole file is 
> >   export GYP_DEFINES="skia_asan_build=1 ${GYP_DEFINES}"
> > 
> > That is, it reminds me the thing I need to do turn on asan.  Maybe instead
of
> > foo_build files, we can have a build_options.readme file?
> >   CXX=foo                            // use foo to compile C++ code
> >   skia_arch_width=[32,64]   // controls compiler -m flag
> >   skia_clang_build=1           // enable extra warning flags only Clang
knows
> > about
> >   skia_gpu=0                       // disable GPU support all over the
> codebase
> >   skia_asan_build=1            // builds with asan, requires GCC >= 4.8 or
> Clang
> > >= 3.2
> >   skia_tsan_build=1             // builds with tsan, requires ....
> > etc ?
> 
> I was under the impression that the vast majority were using make.  If such a
> readme would be helpful, then we can certainly add it.  The asan_build script
> primarily exists to set those variables, and if they didn't need to be set,
> adding the gyp_define would be sufficient.  I propose that, if we're going to
> add that readme, it should be auto-generated from gyp/common_variables.gypi. 
Or
> maybe common_variables just needs better formatting/commenting so that it can
> serve as the build readme.

Yeah, good call.  Let's do the commenting + formatting in common_variables.

[bungeman-skia, 2013-09-25 22:39:28]

Committed patchset #1 manually as r11464.