Keyboard Shortcuts
File
u :up to issue
m :publish + mail comments
M :edit review message
j / k :jump to file after / before current file
J / K :jump to next file with a comment after / before current file
Side-by-side diff
i :toggle intra-line diffs
e :expand all comments
c :collapse all comments
s :toggle showing all comments
n / p :next / previous diff chunk or comment
N / P :next / previous comment
<Up> / <Down> :next / previous line
<Enter> :respond to / edit current comment
d :mark current comment as done
Issue
u :up to list of issues
m :publish + mail comments
j / k :jump to patch after / before current patch
o / <Enter> :open current patch in side-by-side view
i :open current patch in unified diff view
Issue List
j / k :jump to issue after / before current issue
o / <Enter> :open current issue
# : close issue
Comment/message editing
<Ctrl> + s or <Ctrl> + Enter :save comment
<Esc> :cancel edit
Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(99)
Issues Repositories Search
Open Issues | Closed Issues | All Issues | Sign in with your Google Account to create issues and add comments

Issue 1708042: A lot of stuff.

Can't Edit
Can't Publish+Mail
Start Review
Created:
15 years, 7 months ago by Evgeniy Stepanov
Modified:
15 years, 7 months ago
Reviewers:
glider, kcc
Visibility:
Public.

Patch Set 1 #

Patch Set 2 : Restored the DCHECK that in phb mode intersection of LSs is empty. #

Patch Set 3 : z #

Total comments: 14

Patch Set 4 : z #

Patch Set 5 : z #

Total comments: 8

Patch Set 6 : z #

Patch Set 7 : z #

Created: 15 years, 7 months ago
Download [raw] [tar.bz2]
Unified diffs Side-by-side diffs Delta from patch set Stats (+355 lines, -188 lines) Patch
M dynamic_annotations/dynamic_annotations.h View 1 2 3 4 5 13 chunks +142 lines, -102 lines 0 comments Download
M dynamic_annotations/dynamic_annotations.c View 1 2 3 4 1 chunk +73 lines, -67 lines 0 comments Download
M tsan/thread_sanitizer.h View 1 2 3 4 5 6 2 chunks +10 lines, -0 lines 0 comments Download
M tsan/thread_sanitizer.cc View 1 2 3 4 5 6 12 chunks +60 lines, -10 lines 0 comments Download
M tsan/ts_events.h View 1 chunk +1 line, -0 lines 0 comments Download
M tsan/ts_pin.cc View 2 chunks +7 lines, -0 lines 0 comments Download
M tsan/ts_valgrind.cc View 1 2 3 4 5 6 5 chunks +24 lines, -8 lines 0 comments Download
M tsan/ts_valgrind_client_requests.h View 1 chunk +2 lines, -1 line 0 comments Download
M tsan/ts_valgrind_intercepts.c View 1 chunk +7 lines, -0 lines 0 comments Download
M unittest/racecheck_unittest.cc View 3 4 5 1 chunk +29 lines, -0 lines 0 comments Download
Total messages: 6
|
Evgeniy Stepanov
This CL does 3 things: - Adds a new annotation that tells TSan that a ...
15 years, 7 months ago (2010年06月16日 12:48:26 UTC) #1 
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.
Sign in to reply to this message.
kcc
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 ...
15 years, 7 months ago (2010年06月18日 11:28:45 UTC) #2 
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
Sign in to reply to this message.
Evgeniy Stepanov
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, ...
15 years, 7 months ago (2010年06月18日 11:50:15 UTC) #3 
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.
Sign in to reply to this message.
kcc
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... ...
15 years, 7 months ago (2010年06月18日 12:05:05 UTC) #4 
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
Sign in to reply to this message.
kcc
LGTM Please make sure that 32-bit build does not have warnings
15 years, 7 months ago (2010年06月18日 12:41:53 UTC) #5 
LGTM
Please make sure that 32-bit build does not have warnings
Sign in to reply to this message.
Evgeniy Stepanov
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: > ...
15 years, 7 months ago (2010年06月18日 12:53:22 UTC) #6 
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.
Sign in to reply to this message.
|
Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b

AltStyle によって変換されたページ (->オリジナル) /