A lot of stuff.

[Evgeniy Stepanov, 2010-06-16 12:48:26]

This CL does 3 things:
- Adds a new annotation that tells TSan that a particular mutex is not PHB - so
that it would not create extra H-B arcs.
- Supports custom prefixes for annotation names. This helps against standard
annotation interceptors intercepting annotations in untrusted NaCl code.
- Supports separate tracing of trusted and untrusted parts of NaCl.

[kcc, 2010-06-18 11:28:45]

http://codereview.appspot.com/1708042/diff/5001/4009
File dynamic_annotations/dynamic_annotations.c (right):

http://codereview.appspot.com/1708042/diff/5001/4009#newcode59
dynamic_annotations/dynamic_annotations.c:59: void
DYNAMIC_ANNOTATIONS_NAME(AnnotateRWLockReleased)(const char *file, int line,
80 chars per line, please

http://codereview.appspot.com/1708042/diff/5001/4010
File dynamic_annotations/dynamic_annotations.h (right):

http://codereview.appspot.com/1708042/diff/5001/4010#newcode143
dynamic_annotations/dynamic_annotations.h:143:
DYNAMIC_ANNOTATIONS_NAME(AnnotatePublishMemoryRange)(__FILE__, __LINE__,
pointer, size)
80 chars

http://codereview.appspot.com/1708042/diff/5001/4010#newcode166
dynamic_annotations/dynamic_annotations.h:166: #define
ANNOTATE_NON_PURE_HAPPENS_BEFORE_MUTEX(mu) \
comment:
Opposite to ANNOTATE_PURE_HAPPENS_BEFORE_MUTEX. 
Instruct the tool to NOT create h-b arcs between Unlock and Lock, even in pure
happens-before mode. For a hybrid mode this is a no-op. 

Maybe name it ANNOTATE_NOT_HAPPENS_BEFORE_MUTEX? Hm...

http://codereview.appspot.com/1708042/diff/5001/4011
File tsan/thread_sanitizer.cc (right):

http://codereview.appspot.com/1708042/diff/5001/4011#newcode5565
tsan/thread_sanitizer.cc:5565: //          
DCHECK(!G_flags->pure_happens_before);
???

http://codereview.appspot.com/1708042/diff/5001/4011#newcode5579
tsan/thread_sanitizer.cc:5579: //          
DCHECK(!G_flags->pure_happens_before);
???

http://codereview.appspot.com/1708042/diff/5001/4011#newcode6679
tsan/thread_sanitizer.cc:6679: return addr < g_nacl_mem_start;
wrong, you need to check the range [nacl_mem_start, nacl_mem_start + 4G)

http://codereview.appspot.com/1708042/diff/5001/4015
File tsan/ts_valgrind.cc (right):

http://codereview.appspot.com/1708042/diff/5001/4015#newcode563
tsan/ts_valgrind.cc:563: static inline Bool ignoring_sync(ThreadId vg_tid,
uintptr_t addr) {
comment, please

[Evgeniy Stepanov, 2010-06-18 11:50:15]

http://codereview.appspot.com/1708042/diff/5001/4009
File dynamic_annotations/dynamic_annotations.c (right):

http://codereview.appspot.com/1708042/diff/5001/4009#newcode59
dynamic_annotations/dynamic_annotations.c:59: void
DYNAMIC_ANNOTATIONS_NAME(AnnotateRWLockReleased)(const char *file, int line,
On 2010/06/18 11:28:46, kcc wrote:
> 80 chars per line, please

Done.

http://codereview.appspot.com/1708042/diff/5001/4010
File dynamic_annotations/dynamic_annotations.h (right):

http://codereview.appspot.com/1708042/diff/5001/4010#newcode143
dynamic_annotations/dynamic_annotations.h:143:
DYNAMIC_ANNOTATIONS_NAME(AnnotatePublishMemoryRange)(__FILE__, __LINE__,
pointer, size)
On 2010/06/18 11:28:46, kcc wrote:
> 80 chars

Done.

http://codereview.appspot.com/1708042/diff/5001/4010#newcode166
dynamic_annotations/dynamic_annotations.h:166: #define
ANNOTATE_NON_PURE_HAPPENS_BEFORE_MUTEX(mu) \
On 2010/06/18 11:28:46, kcc wrote:
> comment:
> Opposite to ANNOTATE_PURE_HAPPENS_BEFORE_MUTEX. 
> Instruct the tool to NOT create h-b arcs between Unlock and Lock, even in pure
> happens-before mode. For a hybrid mode this is a no-op. 
> 
> Maybe name it ANNOTATE_NOT_HAPPENS_BEFORE_MUTEX? Hm... 

Done.

http://codereview.appspot.com/1708042/diff/5001/4011
File tsan/thread_sanitizer.cc (right):

http://codereview.appspot.com/1708042/diff/5001/4011#newcode5565
tsan/thread_sanitizer.cc:5565: //          
DCHECK(!G_flags->pure_happens_before);
On 2010/06/18 11:28:46, kcc wrote:
> ???

Done.

http://codereview.appspot.com/1708042/diff/5001/4011#newcode5579
tsan/thread_sanitizer.cc:5579: //          
DCHECK(!G_flags->pure_happens_before);
On 2010/06/18 11:28:46, kcc wrote:
> ???

Done.

http://codereview.appspot.com/1708042/diff/5001/4011#newcode6679
tsan/thread_sanitizer.cc:6679: return addr < g_nacl_mem_start;
On 2010/06/18 11:28:46, kcc wrote:
> wrong, you need to check the range [nacl_mem_start, nacl_mem_start + 4G)

Done.

http://codereview.appspot.com/1708042/diff/5001/4015
File tsan/ts_valgrind.cc (right):

http://codereview.appspot.com/1708042/diff/5001/4015#newcode563
tsan/ts_valgrind.cc:563: static inline Bool ignoring_sync(ThreadId vg_tid,
uintptr_t addr) {
On 2010/06/18 11:28:46, kcc wrote:
> comment, please

Done.

[kcc, 2010-06-18 12:05:05]

http://codereview.appspot.com/1708042/diff/14001/15002
File dynamic_annotations/dynamic_annotations.h (right):

http://codereview.appspot.com/1708042/diff/14001/15002#newcode172
dynamic_annotations/dynamic_annotations.h:172: #define
ANNOTATE_NON_PURE_HAPPENS_BEFORE_MUTEX(mu) \
Bad name, imho. Naming is hard... 
ANNOTATE_PURE_LOCKSET_MUTEX? 
ANNOTATE_NO_HAPPENS_BEFORE_MUTEX?

http://codereview.appspot.com/1708042/diff/14001/15003
File tsan/thread_sanitizer.cc (right):

http://codereview.appspot.com/1708042/diff/14001/15003#newcode5576
tsan/thread_sanitizer.cc:5576: } else if (DEBUG_MODE) {
why need DEBUG_MODE given the DHECK?

http://codereview.appspot.com/1708042/diff/14001/15003#newcode6696
tsan/thread_sanitizer.cc:6696: return addr < g_nacl_mem_start || addr >=
g_nacl_mem_start + 0x100000000ULL;
What happens if g_nacl_mem_start is still -1? 
Please make a function like AddrIsInNaclUntrustedRegion and call it from here. 
Also, name the magic const something like kFourGig

http://codereview.appspot.com/1708042/diff/14001/15007
File tsan/ts_valgrind.cc (right):

http://codereview.appspot.com/1708042/diff/14001/15007#newcode562
tsan/ts_valgrind.cc:562: // Whether we are currently ignoring sync events for
the given thread at the given
80 chars

[kcc, 2010-06-18 12:41:53]

LGTM
Please make sure that 32-bit build does not have warnings

[Evgeniy Stepanov, 2010-06-18 12:53:22]

http://codereview.appspot.com/1708042/diff/14001/15002
File dynamic_annotations/dynamic_annotations.h (right):

http://codereview.appspot.com/1708042/diff/14001/15002#newcode172
dynamic_annotations/dynamic_annotations.h:172: #define
ANNOTATE_NON_PURE_HAPPENS_BEFORE_MUTEX(mu) \
On 2010/06/18 12:05:05, kcc wrote:
> Bad name, imho. Naming is hard... 
> ANNOTATE_PURE_LOCKSET_MUTEX? 
> ANNOTATE_NO_HAPPENS_BEFORE_MUTEX? 

ANNOTATE_NOT_HAPPENS_BEFORE_MUTEX

http://codereview.appspot.com/1708042/diff/14001/15003
File tsan/thread_sanitizer.cc (right):

http://codereview.appspot.com/1708042/diff/14001/15003#newcode5576
tsan/thread_sanitizer.cc:5576: } else if (DEBUG_MODE) {
On 2010/06/18 12:05:05, kcc wrote:
> why need DEBUG_MODE given the DHECK? 

Indeed.

http://codereview.appspot.com/1708042/diff/14001/15003#newcode6696
tsan/thread_sanitizer.cc:6696: return addr < g_nacl_mem_start || addr >=
g_nacl_mem_start + 0x100000000ULL;
On 2010/06/18 12:05:05, kcc wrote:
> What happens if g_nacl_mem_start is still -1? 
> Please make a function like AddrIsInNaclUntrustedRegion and call it from here.

> Also, name the magic const something like kFourGig

Done.

http://codereview.appspot.com/1708042/diff/14001/15007
File tsan/ts_valgrind.cc (right):

http://codereview.appspot.com/1708042/diff/14001/15007#newcode562
tsan/ts_valgrind.cc:562: // Whether we are currently ignoring sync events for
the given thread at the given
On 2010/06/18 12:05:05, kcc wrote:
> 80 chars

Done.