|
|
|
Created:
15 years, 4 months ago by ramosian.glider Modified:
15 years, 3 months ago Base URL:
http://data-race-test.googlecode.com/svn/trunk/tsan/ Visibility:
Public. |
Patch Set 1 #
Total comments: 16
Patch Set 2 : Fixed kcc@ and eugenis@'s comments for patch set 1 #
Total comments: 4
Patch Set 3 : Fixed the IgnoreTriple initialization #
Total comments: 4
Patch Set 4 : Fixed Timur's comments from patch set 3 #
Total comments: 3
Patch Set 5 : Fixed output parameter, removed fun_hist (kcc's comments) #
Total comments: 5
Patch Set 6 : fixed Timur's comments from PS5 #
Total comments: 1
Patch Set 7 : Fixed the CHECK in IgnoreTriple ctor #Patch Set 8 : Fixed the matcher after running the tests #Patch Set 9 : updated the comment in MatchKnown #Total messages: 15
|
ramosian.glider
|
15 years, 4 months ago (2010年09月13日 08:38:26 UTC) #1 |
http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode6955 thread_sanitizer.cc:6955: struct IgnoreTriple { add some comments please http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode6977 thread_sanitizer.cc:6977: vector<IgnoreTriple> funs; "funs" seems like a bad name for this http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7089 thread_sanitizer.cc:7089: ReadIgnoreLine(line, "obj:", g_ignore_lists->funs) || You can just parse the prefix in ReadIgnoreLine instead of calling it 5 times. http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7135 thread_sanitizer.cc:7135: bool TripleVectorPartMatch(const vector<IgnoreTriple>& v, TripleVectorPartialMatch? TripleVectorMatchKnown? http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7184 thread_sanitizer.cc:7184: return !(TripleVectorMatch(g_ignore_lists->funs_hist, rtn_name, "*", "*")); You can not use stars here! It is a string that is matched against a template, not a template itself.
http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode6991 thread_sanitizer.cc:6991: } else { } else if (...) { http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7107 thread_sanitizer.cc:7107: cur = ConvertToPlatformIndependentPath(cur); Do it in CTOR? http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7168 thread_sanitizer.cc:7168: bool ignore = TripleVectorPartMatch(g_ignore_lists->funs, rtn_name, img_name, file_name) || 80 lines
http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode6955 thread_sanitizer.cc:6955: struct IgnoreTriple { On 2010年09月13日 08:58:07, Evgeniy Stepanov wrote: > add some comments please Done. http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode6977 thread_sanitizer.cc:6977: vector<IgnoreTriple> funs; On 2010年09月13日 08:58:07, Evgeniy Stepanov wrote: > "funs" seems like a bad name for this Fixed. http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7089 thread_sanitizer.cc:7089: ReadIgnoreLine(line, "obj:", g_ignore_lists->funs) || On 2010年09月13日 08:58:07, Evgeniy Stepanov wrote: > You can just parse the prefix in ReadIgnoreLine instead of calling it 5 times. Done. http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7135 thread_sanitizer.cc:7135: bool TripleVectorPartMatch(const vector<IgnoreTriple>& v, On 2010年09月13日 08:58:07, Evgeniy Stepanov wrote: > TripleVectorPartialMatch? > TripleVectorMatchKnown? TripleVectorMatchKnown. Please note a change to the function's implementation. http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7184 thread_sanitizer.cc:7184: return !(TripleVectorMatch(g_ignore_lists->funs_hist, rtn_name, "*", "*")); On 2010年09月13日 08:58:07, Evgeniy Stepanov wrote: > You can not use stars here! It is a string that is matched against a template, > not a template itself. Fixed, thanks!
http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode6991 thread_sanitizer.cc:6991: } else { On 2010年09月13日 10:39:04, kcc wrote: > } else if (...) { Refactored, please see ReadIgnoreLine() http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7107 thread_sanitizer.cc:7107: cur = ConvertToPlatformIndependentPath(cur); On 2010年09月13日 10:39:04, kcc wrote: > Do it in CTOR? Done. http://codereview.appspot.com/2127047/diff/1/thread_sanitizer.cc#newcode7168 thread_sanitizer.cc:7168: bool ignore = TripleVectorPartMatch(g_ignore_lists->funs, rtn_name, img_name, file_name) || On 2010年09月13日 10:39:04, kcc wrote: > 80 lines Done.
http://codereview.appspot.com/2127047/diff/8001/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/8001/thread_sanitizer.cc#newcode6972 thread_sanitizer.cc:6972: obj = ConvertToPlatformIndependentPath(iobj); ConvertToPlatformIndependentPath is only applied to file paths http://codereview.appspot.com/2127047/diff/8001/thread_sanitizer.cc#newcode6973 thread_sanitizer.cc:6973: file = ConvertToPlatformIndependentPath(file); ifile
http://codereview.appspot.com/2127047/diff/8001/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/8001/thread_sanitizer.cc#newcode6972 thread_sanitizer.cc:6972: obj = ConvertToPlatformIndependentPath(iobj); On 2010年09月14日 10:48:59, Evgeniy Stepanov wrote: > ConvertToPlatformIndependentPath is only applied to file paths It was a mistake, I guess. Objects are file names, too. http://codereview.appspot.com/2127047/diff/8001/thread_sanitizer.cc#newcode6973 thread_sanitizer.cc:6973: file = ConvertToPlatformIndependentPath(file); On 2010年09月14日 10:48:59, Evgeniy Stepanov wrote: > ifile Done.
http://codereview.appspot.com/2127047/diff/13001/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/13001/thread_sanitizer.cc#newcode7002 thread_sanitizer.cc:7002: if (input_line.find("src:") == 0) { I'd replace something with a macro or a function. http://codereview.appspot.com/2127047/diff/13001/thread_sanitizer.cc#newcode7134 thread_sanitizer.cc:7134: (file.size() == 0 || StringMatch(v[i].file, file))) return true; "return true" on a separate line, probably with {}
http://codereview.appspot.com/2127047/diff/13001/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/13001/thread_sanitizer.cc#newcode7002 thread_sanitizer.cc:7002: if (input_line.find("src:") == 0) { On 2010年09月14日 12:33:20, timurrrr_at_google_com wrote: > I'd replace something with a macro or a function. Done. http://codereview.appspot.com/2127047/diff/13001/thread_sanitizer.cc#newcode7134 thread_sanitizer.cc:7134: (file.size() == 0 || StringMatch(v[i].file, file))) return true; On 2010年09月14日 12:33:20, timurrrr_at_google_com wrote: > "return true" on a separate line, probably with {} Done.
http://codereview.appspot.com/2127047/diff/18001/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/18001/thread_sanitizer.cc#newcode6997 thread_sanitizer.cc:6997: /* OUT */ string &output) { string * http://codereview.appspot.com/2127047/diff/18001/thread_sanitizer.cc#newcode7169 thread_sanitizer.cc:7169: TripleVectorMatchKnown(g_ignore_lists->ignores_hist, wtf?
http://codereview.appspot.com/2127047/diff/18001/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/18001/thread_sanitizer.cc#newcode6997 thread_sanitizer.cc:6997: /* OUT */ string &output) { On 2010年09月15日 08:12:41, kcc wrote: > string * Fixed
General comment: please run the old and new binaries with --debug_phase=ignores on Mac/media_unittests and compare the logs http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc#newcode6972 thread_sanitizer.cc:6972: file = ConvertToPlatformIndependentPath(ifile); I'd add an assert that all three params can't be "*" at the same time. http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc#newcode6996 thread_sanitizer.cc:6996: bool CutStringPrefix(const string &input, const string &prefix, CutStringPrefixIfPresent ? http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc#newcode7135 thread_sanitizer.cc:7135: if ((fun.size() == 0 || StringMatch(v[i].fun, fun)) && Possible performance issue: default value for 2 of 3 vars are "*", not "".
The third comment was fixed too, but a server error occured while sending that comment http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc#newcode6972 thread_sanitizer.cc:6972: file = ConvertToPlatformIndependentPath(ifile); On 2010年09月15日 10:51:38, timurrrr_at_google_com wrote: > I'd add an assert that all three params can't be "*" at the same time. Done. http://codereview.appspot.com/2127047/diff/21001/thread_sanitizer.cc#newcode7135 thread_sanitizer.cc:7135: if ((fun.size() == 0 || StringMatch(v[i].fun, fun)) && On 2010年09月15日 10:51:38, timurrrr_at_google_com wrote: > Possible performance issue: default value for 2 of 3 vars are "*", not "". Discussed offline
http://codereview.appspot.com/2127047/diff/25001/thread_sanitizer.cc File thread_sanitizer.cc (right): http://codereview.appspot.com/2127047/diff/25001/thread_sanitizer.cc#newcode6973 thread_sanitizer.cc:6973: CHECK(!((ifun == iobj) && (iobj == ifile))); wrong condition (discussed offline)
I've fixed the matcher routine after running the tests, eerything is ok now. Going to land the CL on Monday