|
|
|
Created:
15 years, 7 months ago by kcc Modified:
15 years, 7 months ago Reviewers:
Timur Iskhodzhanov Base URL:
http://data-race-test.googlecode.com/svn/trunk/tsan/ Visibility:
Public. |
Patch Set 1 #Patch Set 2 : fix 80 char #
Total comments: 16
Patch Set 3 : more comments, changed name of stats #Patch Set 4 : less indents #Patch Set 5 : more comments #Patch Set 6 : x #Total messages: 2
|
kcc
|
15 years, 7 months ago (2010年05月26日 07:41:14 UTC) #1 |
http://codereview.appspot.com/1316041/diff/2001/3001 File thread_sanitizer.cc (right): http://codereview.appspot.com/1316041/diff/2001/3001#newcode193 thread_sanitizer.cc:193: LID Singleton() const { return LID(raw()); } GetSingleton() ? http://codereview.appspot.com/1316041/diff/2001/3001#newcode212 thread_sanitizer.cc:212: SID Singleton() const { GetSingleton() ? http://codereview.appspot.com/1316041/diff/2001/3001#newcode5656 thread_sanitizer.cc:5656: // Tuple segment sets and check for race. // Returns true iff bla-bla-bla, writes the result to *new_sval http://codereview.appspot.com/1316041/diff/2001/3001#newcode5661 thread_sanitizer.cc:5661: //#define MSM_STAT(i) G_stats->msm[i]++; do { if (DEBUG_MODE) ...; } while(0) ?? http://codereview.appspot.com/1316041/diff/2001/3001#newcode5665 thread_sanitizer.cc:5665: if (rd_ssid.IsEmpty()) { I think I'd be good to insert the following to the beginning of the function: if (!rd_ssid.IsSingleton() || !wr_ssid.IsSingleton()) return false; http://codereview.appspot.com/1316041/diff/2001/3001#newcode5670 thread_sanitizer.cc:5670: MSM_STAT(0); FastPath1 vs msm[0] ? http://codereview.appspot.com/1316041/diff/2001/3001#newcode5673 thread_sanitizer.cc:5673: // wr_ssid != 0 DCHECK ? http://codereview.appspot.com/1316041/diff/2001/3001#newcode5674 thread_sanitizer.cc:5674: if (wr_ssid.IsSingleton()) { if (!wr_ssid.IsSingleton()) return false; ... to avoid extra indent http://codereview.appspot.com/1316041/diff/2001/3001#newcode5683 thread_sanitizer.cc:5683: // same thread, but different segments. .. segment. http://codereview.appspot.com/1316041/diff/2001/3001#newcode5698 thread_sanitizer.cc:5698: } else { DCHECK(!rd_ssid.IsEmpty()); http://codereview.appspot.com/1316041/diff/2001/3001#newcode5699 thread_sanitizer.cc:5699: if (wr_ssid.IsEmpty()) { invert the condition to get rid of extra indent http://codereview.appspot.com/1316041/diff/2001/3001#newcode5701 thread_sanitizer.cc:5701: if (rd_ssid.IsSingleton()) { if (!rd_ssid.IsSingleton()) return false; http://codereview.appspot.com/1316041/diff/2001/3001#newcode5750 thread_sanitizer.cc:5750: ShadowValue *sval_p = NULL; new_sval_p http://codereview.appspot.com/1316041/diff/2001/3001#newcode5757 thread_sanitizer.cc:5757: } what do you think about "GetOrAddSvalAtOffset" ? http://codereview.appspot.com/1316041/diff/2001/3001#newcode5762 thread_sanitizer.cc:5762: if (!MemoryStateMachineSameThread(is_w, old_sval, tid, hm... is it publish-friendly? http://codereview.appspot.com/1316041/diff/2001/3002 File ts_stats.h (right): http://codereview.appspot.com/1316041/diff/2001/3002#newcode216 ts_stats.h:216: uintptr_t msm[16]; more descriptive name / comment what these stat means.