Add presubmit checks for TSan suppressions

tools/valgrind/suppressions.py

 #!/usr/bin/env python
 # Copyright (c) 2011 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 # suppressions.py
 
 """Post-process Valgrind suppression matcher.
 
 Suppressions are defined as follows:
@@ skipping 21 matching lines @@
 
 class Suppression(object):
   """This class represents a single stack trace suppression.
 
   Attributes:
     description: A string representing the error description.
     type: A string representing the error type, e.g. Memcheck:Leak.
     stack: a list of "fun:" or "obj:" or ellipsis lines.
   """
 
-  def __init__(self, description, type, stack):
+  def __init__(self, description, type, stack, defined_at):
     """Inits Suppression.
 
-    Args: Same as class attributes.
+    description, type, stack: same as class attributes
+    defined_at: file:line identifying where the suppression was defined
     """
     self.description = description
     self.type = type
     self._stack = stack
+    self.defined_at = defined_at
     re_line = '{\n.*\n%s\n' % self.type
     re_bucket = ''
     for line in stack:
       if line == ELLIPSIS:
         re_line += re.escape(re_bucket)
         re_bucket = ''
         re_line += '(.*\n)*'
       else:
         for char in line:
           if char == '*':
@@ skipping 60 matching lines @@
     if not self._stack:
       return False
     lines = [f.strip() for f in suppression_from_report]
     if self._re.match('\n'.join(lines) + '\n'):
       return True
     else:
       return False
 
 
 class SuppressionError(Exception):
-  def __init__(self, filename, line, message=''):
-    Exception.__init__(self, filename, line, message)
-    self._file = filename
-    self._line = line
+  def __init__(self, message, happened_at):
     self._message = message
+    self._happened_at = happened_at
 
   def __str__(self):
-    return 'Error reading suppressions from "%s" (line %d): %s.' % (
-        self._file, self._line, self._message)
+    return 'Error reading suppressions at %s!\n%s' % (
+        self._happened_at, self._message)
 
 def ReadSuppressionsFromFile(filename):
   """Read suppressions from the given file and return them as a list"""
   input_file = file(filename, 'r')
   try:
     return ReadSuppressions(input_file, filename)
   except SuppressionError:
     input_file.close()
     raise
 
 def ReadSuppressions(lines, supp_descriptor):
   """Given a list of lines, returns a list of suppressions.
 
   Args:
     lines: a list of lines containing suppressions.
     supp_descriptor: should typically be a filename.
-      Used only when parsing errors happen.
+        Used only when printing errors.
   """
   result = []
   cur_descr = ''
   cur_type = ''
   cur_stack = []
   in_suppression = False
   nline = 0
   for line in lines:
     nline += 1
     line = line.strip()
     if line.startswith('#'):
       continue
     if not in_suppression:
       if not line:
         # empty lines between suppressions
         pass
       elif line.startswith('{'):
         in_suppression = True
         pass
       else:
-        raise SuppressionError(supp_descriptor, nline,
-                               'Expected: "{"')
+        raise SuppressionError('Expected: "{"',
+                               "%s:%d" % (supp_descriptor, nline))
     elif line.startswith('}'):
-      result.append(Suppression(cur_descr, cur_type, cur_stack))
+      result.append(
+          Suppression(cur_descr, cur_type, cur_stack,
+                      "%s:%d" % (supp_descriptor, nline)))
       cur_descr = ''
       cur_type = ''
       cur_stack = []
       in_suppression = False
     elif not cur_descr:
       cur_descr = line
       continue
     elif not cur_type:
       if (not line.startswith("Memcheck:")) and \
          (not line.startswith("ThreadSanitizer:")) and \
          (line != "Heapcheck:Leak"):
-        raise SuppressionError(supp_descriptor, nline,
-            '"Memcheck:TYPE" , "ThreadSanitizer:TYPE" or "Heapcheck:Leak '
-            'is expected, got "%s"' % line)
+        raise SuppressionError(
+            'Expected "Memcheck:TYPE", "ThreadSanitizer:TYPE" '
+            'or "Heapcheck:Leak", got "%s"' % line,
+            "%s:%d" % (supp_descriptor, nline))
       supp_type = line.split(':')[1]
       if not supp_type in ["Addr1", "Addr2", "Addr4", "Addr8",
                            "Cond", "Free", "Jump", "Leak", "Overlap", "Param",
                            "Value1", "Value2", "Value4", "Value8",
                            "Race", "UnlockNonLocked", "InvalidLock",
                            "Unaddressable", "Uninitialized"]:
-        raise SuppressionError(supp_descriptor, nline,
-                               'Unknown suppression type "%s"' % supp_type)
+        raise SuppressionError('Unknown suppression type "%s"' % supp_type,
+                               "%s:%d" % (supp_descriptor, nline))
       cur_type = line
       continue
     elif re.match("^fun:.*|^obj:.*|^\.\.\.$", line):
       cur_stack.append(line.strip())
     elif len(cur_stack) == 0 and cur_type == "Memcheck:Param":
       cur_stack.append(line.strip())
     else:
-      raise SuppressionError(supp_descriptor, nline,
-                             '"fun:function_name" or "obj:object_file" ' \
-                             'or "..." expected')
+      raise SuppressionError(
+          '"fun:function_name" or "obj:object_file" or "..." expected',
+          "%s:%d" % (supp_descriptor, nline))
   return result
 
 
+def PresubmitCheck(input_api, output_api):
+  """A helper function useful in PRESUBMIT.py
+     Returns a list of errors or [].
+  """
+  sup_regex = re.compile('suppressions.*\.txt$')
+  filenames = [f.AbsoluteLocalPath() for f in input_api.AffectedFiles()
+                   if sup_regex.search(f.LocalPath())]
+
+  errors = []
+
+  # TODO(timurrrr): warn on putting suppressions into a wrong file,
+  # e.g. TSan suppression in a memcheck file.
+
+  for f in filenames:
+    try:
+      known_supp_names = {}  # Key: name, Value: suppression.
+      supps = ReadSuppressionsFromFile(f)
+      for s in supps:
+        if re.search("<.*suppression.name.here>", s.description):
+          # Suppression name line is
+          # <insert_a_suppression_name_here> for Memcheck,
+          # <Put your suppression name here> for TSan,
+          # name=<insert_a_suppression_name_here> for DrMemory
+          errors.append(
+              SuppressionError(
+                  "You've forgotten to put a suppression name like bug_XXX",
+                  s.defined_at))
+          continue
+
+        if s.description in known_supp_names:
+          errors.append(
+              SuppressionError(
+                  'Suppression named "%s" is defined more than once, '
+                  'see %s' % (s.description,
+                              known_supp_names[s.description].defined_at),
+                  s.defined_at))
+        else:
+          known_supp_names[s.description] = s
+
+    except SuppressionError as e:
+      errors.append(e)
+
+  return [output_api.PresubmitError(str(e)) for e in errors]
+
+
 def TestStack(stack, positive, negative):
   """A helper function for SelfTest() that checks a single stack.
 
   Args:
     stack: the stack to match the suppressions.
     positive: the list of suppressions that must match the given stack.
     negative: the list of suppressions that should not match.
   """
   for supp in positive:
-    assert ReadSuppressions(supp.split("\n"), "")[0].Match(stack), \
-           "Suppression:\n%s\ndidn't match stack:\n%s" % (supp, stack)
+    parsed = ReadSuppressions(supp.split("\n"), "positive_suppression")
+    assert parsed[0].Match(stack), \
+        "Suppression:\n%s\ndidn't match stack:\n%s" % (supp, stack)
   for supp in negative:
-    assert not ReadSuppressions(supp.split("\n"), "")[0].Match(stack), \
-           "Suppression:\n%s\ndid match stack:\n%s" % (supp, stack)
+    parsed = ReadSuppressions(supp.split("\n"), "negative_suppression")
+    assert not parsed[0].Match(stack), \
+        "Suppression:\n%s\ndid match stack:\n%s" % (supp, stack)
 

[Alexander Potapenko, 2011/12/15 13:35:31]

Please add a blank line, as this is a top-level definition.

 def SelfTest():
   """Tests the Suppression.Match() capabilities."""
 
   test_memcheck_stack_1 = """{
     test
     Memcheck:Leak
     fun:absolutly
     fun:brilliant
     obj:condition
     fun:detection
@@ skipping 162 matching lines @@
             positive_memcheck_suppressions_4,
             negative_memcheck_suppressions_4)
   TestStack(test_heapcheck_stack, positive_heapcheck_suppressions,
             negative_heapcheck_suppressions)
   TestStack(test_tsan_stack, positive_tsan_suppressions,
             negative_tsan_suppressions)
 
 if __name__ == '__main__':
   SelfTest()
   print 'PASS'