Refactor "Suppressions used" printing code in memcheck and tsan analyzer scripts

Refactor "Suppressions used" printing code in memcheck and tsan analyzer scripts
The code was added yesterday while improving drmemory_analyze.py, we want to
have exactly the same format of used suppressions to simplify the code of the
suppression dashboard parser

TBR=glider
TEST=./tools/valgrind/chrome_tests.sh --tool {tsan,memcheck} -t base --gtest_filter="*Sanity*"
     with and without the suppression files
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111580

[Alexander Potapenko, 2011-11-25 15:03:11]

Please take a look:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20%28...

On Fri, Nov 25, 2011 at 2:06 PM,  <timurrrr@chromium.org> wrote:
> Reviewers: Alexander Potapenko,
>
> Description:
> Refactor "Suppressions used" printing code in memcheck and tsan analyzer
> scripts
> The code was added yesterday while improving drmemory_analyze.py, we want to
> have exactly the same format of used suppressions to simplify the code of
> the
> suppression dashboard parser
>
> TBR=glider
> TEST=./tools/valgrind/chrome_tests.sh --tool {tsan,memcheck} -t base
> --gtest_filter="*Sanity*"
>     with and without the suppression files
>
> Please review this at http://codereview.chromium.org/8702004/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     tools/valgrind/memcheck_analyze.py
>  M     tools/valgrind/tsan/suppressions.txt
>  M     tools/valgrind/tsan_analyze.py
>
>
> Index: tools/valgrind/memcheck_analyze.py
> ===================================================================
> --- tools/valgrind/memcheck_analyze.py  (revision 111579)
> +++ tools/valgrind/memcheck_analyze.py  (working copy)
> @@ -9,6 +9,7 @@
>
>  import gdb_helper
>
> +from collections import defaultdict
>  import hashlib
>  import logging
>  import optparse
> @@ -435,7 +436,7 @@
>     else:
>       TheAddressTable = None
>     cur_report_errors = set()
> -    suppcounts = {}
> +    suppcounts = defaultdict(int)
>     badfiles = set()
>
>     if self._analyze_start_time == None:
> @@ -548,10 +549,7 @@
>           for node in suppcountlist.getElementsByTagName("pair"):
>             count = getTextOf(node, "count");
>             name = getTextOf(node, "name");
> -            if name in suppcounts:
> -              suppcounts[name] += int(count)
> -            else:
> -              suppcounts[name] = int(count)
> +            suppcounts[name] += int(count)
>
>     if len(badfiles) > 0:
>       logging.warn("valgrind didn't finish writing %d files?!" %
> len(badfiles))
> @@ -563,22 +561,8 @@
>       logging.error("FAIL! Couldn't parse Valgrind output file")
>       return -2
>
> -    is_sane = False
> -    print "-----------------------------------------------------"
> -    print "Suppressions used:"
> -    print "  count name"
> +    common.PrintUsedSuppressionsList(suppcounts)
>
> -    remaining_sanity_supp = MemcheckAnalyzer.SANITY_TEST_SUPPRESSIONS
> -    for (name, count) in sorted(suppcounts.items(),
> -                                key=lambda (k,v): (v,k)):
> -      print "%7d %s" % (count, name)
> -      if name in remaining_sanity_supp and remaining_sanity_supp[name] ==
> count:
> -        del remaining_sanity_supp[name]
> -    if len(remaining_sanity_supp) == 0:
> -      is_sane = True
> -    print "-----------------------------------------------------"
> -    sys.stdout.flush()
> -
>     retcode = 0
>     if cur_report_errors:
>       logging.error("FAIL! There were %s errors: " % len(cur_report_errors))
> @@ -592,12 +576,17 @@
>       retcode = -1
>
>     # Report tool's insanity even if there were errors.
> -    if check_sanity and not is_sane:
> -      logging.error("FAIL! Sanity check failed!")
> -      logging.info("The following test errors were not handled: ")
> -      for (name, count) in sorted(remaining_sanity_supp.items(),
> -                                  key=lambda (k,v): (v,k)):
> -        logging.info("%7d %s" % (count, name))
> +    if check_sanity:
> +      remaining_sanity_supp = MemcheckAnalyzer.SANITY_TEST_SUPPRESSIONS
> +      for (name, count) in suppcounts.iteritems():
> +        if (name in remaining_sanity_supp and
> +            remaining_sanity_supp[name] == count):
> +          del remaining_sanity_supp[name]
> +      if remaining_sanity_supp:
> +        logging.error("FAIL! Sanity check failed!")
> +        logging.info("The following test errors were not handled: ")
> +        for (name, count) in remaining_sanity_supp.iteritems():
> +          logging.info("  * %dx %s" % (count, name))
>       retcode = -3
>
>     if retcode != 0:
> Index: tools/valgrind/tsan/suppressions.txt
> ===================================================================
> --- tools/valgrind/tsan/suppressions.txt        (revision 111579)
> +++ tools/valgrind/tsan/suppressions.txt        (working copy)
> @@ -330,7 +330,7 @@
>  }
>
>  {
> -  ThreadSanitizer sanity test (ToolsSanityTest.DataRace).
> +  ThreadSanitizer sanity test (ToolsSanityTest.DataRace)
>   ThreadSanitizer:Race
>   fun:*TOOLS_SANITY_TEST_CONCURRENT_THREAD::ThreadMain
>  }
> Index: tools/valgrind/tsan_analyze.py
> ===================================================================
> --- tools/valgrind/tsan_analyze.py      (revision 111579)
> +++ tools/valgrind/tsan_analyze.py      (working copy)
> @@ -9,7 +9,7 @@
>
>  import gdb_helper
>
> -import common
> +from collections import defaultdict
>  import hashlib
>  import logging
>  import optparse
> @@ -19,6 +19,8 @@
>  import sys
>  import time
>
> +import common
> +
>  # Global symbol table (ugh)
>  TheAddressTable = None
>
> @@ -48,7 +50,8 @@
>   THREAD_CREATION_STR = ("INFO: T.* "
>       "(has been created by T.* at this point|is program's main thread)")
>
> -  SANITY_TEST_SUPPRESSION = "ThreadSanitizer sanity test"
> +  SANITY_TEST_SUPPRESSION = ("ThreadSanitizer sanity test "
> +                             "(ToolsSanityTest.DataRace)")
>   TSAN_RACE_DESCRIPTION = "Possible data race"
>   TSAN_WARNING_DESCRIPTION =  ("Unlocking a non-locked lock"
>       "|accessing an invalid lock"
> @@ -186,10 +189,7 @@
>       if match:
>         count, supp_name = match.groups()
>         count = int(count)
> -        if supp_name in self.used_suppressions:
> -          self.used_suppressions[supp_name] += count
> -        else:
> -          self.used_suppressions[supp_name] = count
> +        self.used_suppressions[supp_name] += count
>     self.cur_fd_.close()
>     return ret
>
> @@ -207,7 +207,7 @@
>     else:
>       TheAddressTable = None
>     reports = []
> -    self.used_suppressions = {}
> +    self.used_suppressions = defaultdict(int)
>     for file in files:
>       reports.extend(self.ParseReportFile(file))
>     if self._use_gdb:
> @@ -230,17 +230,9 @@
>     reports = self.GetReports(files)
>     self._cur_testcase = None  # just in case, shouldn't be used anymore
>
> -    is_sane = False
> -    print "-----------------------------------------------------"
> -    print "Suppressions used:"
> -    print "  count name"
> -    for item in sorted(self.used_suppressions.items(), key=lambda (k,v):
> (v,k)):
> -      print "%7s %s" % (item[1], item[0])
> -      if item[0].startswith(TsanAnalyzer.SANITY_TEST_SUPPRESSION):
> -        is_sane = True
> -    print "-----------------------------------------------------"
> -    sys.stdout.flush()
> +    common.PrintUsedSuppressionsList(self.used_suppressions)
>
> +
>     retcode = 0
>     if reports:
>       logging.error("FAIL! Found %i report(s)" % len(reports))
> @@ -249,12 +241,14 @@
>       retcode = -1
>
>     # Report tool's insanity even if there were errors.
> -    if check_sanity and not is_sane:
> +    if (check_sanity and
> +        TsanAnalyzer.SANITY_TEST_SUPPRESSION not in
> self.used_suppressions):
>       logging.error("FAIL! Sanity check failed!")
>       retcode = -3
>
>     if retcode != 0:
>       return retcode
> +
>     logging.info("PASS: No reports found")
>     return 0
>
>
>
>



-- 
Alexander Potapenko
Software Engineer
Google Moscow

[Timur Iskhodzhanov, 2011-11-25 16:42:02]

Thanks for notifying!
Fixed in r111602

On Fri, Nov 25, 2011 at 7:03 PM, Alexander Potapenko
<glider@chromium.org> wrote:
> Please take a look:
>
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Mac%20%28...
>
> On Fri, Nov 25, 2011 at 2:06 PM,  <timurrrr@chromium.org> wrote:
>> Reviewers: Alexander Potapenko,
>>
>> Description:
>> Refactor "Suppressions used" printing code in memcheck and tsan analyzer
>> scripts
>> The code was added yesterday while improving drmemory_analyze.py, we want to
>> have exactly the same format of used suppressions to simplify the code of
>> the
>> suppression dashboard parser
>>
>> TBR=glider
>> TEST=./tools/valgrind/chrome_tests.sh --tool {tsan,memcheck} -t base
>> --gtest_filter="*Sanity*"
>>     with and without the suppression files
>>
>> Please review this at http://codereview.chromium.org/8702004/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>>
>> Affected files:
>>  M     tools/valgrind/memcheck_analyze.py
>>  M     tools/valgrind/tsan/suppressions.txt
>>  M     tools/valgrind/tsan_analyze.py
>>
>>
>> Index: tools/valgrind/memcheck_analyze.py
>> ===================================================================
>> --- tools/valgrind/memcheck_analyze.py  (revision 111579)
>> +++ tools/valgrind/memcheck_analyze.py  (working copy)
>> @@ -9,6 +9,7 @@
>>
>>  import gdb_helper
>>
>> +from collections import defaultdict
>>  import hashlib
>>  import logging
>>  import optparse
>> @@ -435,7 +436,7 @@
>>     else:
>>       TheAddressTable = None
>>     cur_report_errors = set()
>> -    suppcounts = {}
>> +    suppcounts = defaultdict(int)
>>     badfiles = set()
>>
>>     if self._analyze_start_time == None:
>> @@ -548,10 +549,7 @@
>>           for node in suppcountlist.getElementsByTagName("pair"):
>>             count = getTextOf(node, "count");
>>             name = getTextOf(node, "name");
>> -            if name in suppcounts:
>> -              suppcounts[name] += int(count)
>> -            else:
>> -              suppcounts[name] = int(count)
>> +            suppcounts[name] += int(count)
>>
>>     if len(badfiles) > 0:
>>       logging.warn("valgrind didn't finish writing %d files?!" %
>> len(badfiles))
>> @@ -563,22 +561,8 @@
>>       logging.error("FAIL! Couldn't parse Valgrind output file")
>>       return -2
>>
>> -    is_sane = False
>> -    print "-----------------------------------------------------"
>> -    print "Suppressions used:"
>> -    print "  count name"
>> +    common.PrintUsedSuppressionsList(suppcounts)
>>
>> -    remaining_sanity_supp = MemcheckAnalyzer.SANITY_TEST_SUPPRESSIONS
>> -    for (name, count) in sorted(suppcounts.items(),
>> -                                key=lambda (k,v): (v,k)):
>> -      print "%7d %s" % (count, name)
>> -      if name in remaining_sanity_supp and remaining_sanity_supp[name] ==
>> count:
>> -        del remaining_sanity_supp[name]
>> -    if len(remaining_sanity_supp) == 0:
>> -      is_sane = True
>> -    print "-----------------------------------------------------"
>> -    sys.stdout.flush()
>> -
>>     retcode = 0
>>     if cur_report_errors:
>>       logging.error("FAIL! There were %s errors: " % len(cur_report_errors))
>> @@ -592,12 +576,17 @@
>>       retcode = -1
>>
>>     # Report tool's insanity even if there were errors.
>> -    if check_sanity and not is_sane:
>> -      logging.error("FAIL! Sanity check failed!")
>> -      logging.info("The following test errors were not handled: ")
>> -      for (name, count) in sorted(remaining_sanity_supp.items(),
>> -                                  key=lambda (k,v): (v,k)):
>> -        logging.info("%7d %s" % (count, name))
>> +    if check_sanity:
>> +      remaining_sanity_supp = MemcheckAnalyzer.SANITY_TEST_SUPPRESSIONS
>> +      for (name, count) in suppcounts.iteritems():
>> +        if (name in remaining_sanity_supp and
>> +            remaining_sanity_supp[name] == count):
>> +          del remaining_sanity_supp[name]
>> +      if remaining_sanity_supp:
>> +        logging.error("FAIL! Sanity check failed!")
>> +        logging.info("The following test errors were not handled: ")
>> +        for (name, count) in remaining_sanity_supp.iteritems():
>> +          logging.info("  * %dx %s" % (count, name))
>>       retcode = -3
>>
>>     if retcode != 0:
>> Index: tools/valgrind/tsan/suppressions.txt
>> ===================================================================
>> --- tools/valgrind/tsan/suppressions.txt        (revision 111579)
>> +++ tools/valgrind/tsan/suppressions.txt        (working copy)
>> @@ -330,7 +330,7 @@
>>  }
>>
>>  {
>> -  ThreadSanitizer sanity test (ToolsSanityTest.DataRace).
>> +  ThreadSanitizer sanity test (ToolsSanityTest.DataRace)
>>   ThreadSanitizer:Race
>>   fun:*TOOLS_SANITY_TEST_CONCURRENT_THREAD::ThreadMain
>>  }
>> Index: tools/valgrind/tsan_analyze.py
>> ===================================================================
>> --- tools/valgrind/tsan_analyze.py      (revision 111579)
>> +++ tools/valgrind/tsan_analyze.py      (working copy)
>> @@ -9,7 +9,7 @@
>>
>>  import gdb_helper
>>
>> -import common
>> +from collections import defaultdict
>>  import hashlib
>>  import logging
>>  import optparse
>> @@ -19,6 +19,8 @@
>>  import sys
>>  import time
>>
>> +import common
>> +
>>  # Global symbol table (ugh)
>>  TheAddressTable = None
>>
>> @@ -48,7 +50,8 @@
>>   THREAD_CREATION_STR = ("INFO: T.* "
>>       "(has been created by T.* at this point|is program's main thread)")
>>
>> -  SANITY_TEST_SUPPRESSION = "ThreadSanitizer sanity test"
>> +  SANITY_TEST_SUPPRESSION = ("ThreadSanitizer sanity test "
>> +                             "(ToolsSanityTest.DataRace)")
>>   TSAN_RACE_DESCRIPTION = "Possible data race"
>>   TSAN_WARNING_DESCRIPTION =  ("Unlocking a non-locked lock"
>>       "|accessing an invalid lock"
>> @@ -186,10 +189,7 @@
>>       if match:
>>         count, supp_name = match.groups()
>>         count = int(count)
>> -        if supp_name in self.used_suppressions:
>> -          self.used_suppressions[supp_name] += count
>> -        else:
>> -          self.used_suppressions[supp_name] = count
>> +        self.used_suppressions[supp_name] += count
>>     self.cur_fd_.close()
>>     return ret
>>
>> @@ -207,7 +207,7 @@
>>     else:
>>       TheAddressTable = None
>>     reports = []
>> -    self.used_suppressions = {}
>> +    self.used_suppressions = defaultdict(int)
>>     for file in files:
>>       reports.extend(self.ParseReportFile(file))
>>     if self._use_gdb:
>> @@ -230,17 +230,9 @@
>>     reports = self.GetReports(files)
>>     self._cur_testcase = None  # just in case, shouldn't be used anymore
>>
>> -    is_sane = False
>> -    print "-----------------------------------------------------"
>> -    print "Suppressions used:"
>> -    print "  count name"
>> -    for item in sorted(self.used_suppressions.items(), key=lambda (k,v):
>> (v,k)):
>> -      print "%7s %s" % (item[1], item[0])
>> -      if item[0].startswith(TsanAnalyzer.SANITY_TEST_SUPPRESSION):
>> -        is_sane = True
>> -    print "-----------------------------------------------------"
>> -    sys.stdout.flush()
>> +    common.PrintUsedSuppressionsList(self.used_suppressions)
>>
>> +
>>     retcode = 0
>>     if reports:
>>       logging.error("FAIL! Found %i report(s)" % len(reports))
>> @@ -249,12 +241,14 @@
>>       retcode = -1
>>
>>     # Report tool's insanity even if there were errors.
>> -    if check_sanity and not is_sane:
>> +    if (check_sanity and
>> +        TsanAnalyzer.SANITY_TEST_SUPPRESSION not in
>> self.used_suppressions):
>>       logging.error("FAIL! Sanity check failed!")
>>       retcode = -3
>>
>>     if retcode != 0:
>>       return retcode
>> +
>>     logging.info("PASS: No reports found")
>>     return 0
>>
>>
>>
>>
>
>
>
> --
> Alexander Potapenko
> Software Engineer
> Google Moscow
>