[Findit] Plain objects to represent and parse stack trace.

tools/findit/stacktrace.py

Index: tools/findit/stacktrace.py
diff --git a/tools/findit/stacktrace.py b/tools/findit/stacktrace.py
new file mode 100644
index 0000000000000000000000000000000000000000..222615d220bea1a7b528b60ef7ee3ad4dffe5b6c
--- /dev/null
+++ b/tools/findit/stacktrace.py
@@ -0,0 +1,219 @@
+# Copyright (c) 2014 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.
+
+import os
+import re
+
+import crash_utils
+
+
+class Stacktrace(object):
+  """Represents Stacktrace object.
+
+  Contains a list of callstacks, because one stacktrace might have more than
+  one callstacks.
+  """
+
+  def __init__(self, stacktrace, chrome_build):
+    self.stack_list = []
+    self.ParseStacktrace(stacktrace, chrome_build)
+
+  def ParseStacktrace(self, stacktrace, chrome_build):
+    """Parses stacktrace and normalizes it.
+
+    If there are multiple callstacks within the stacktrace,
+    it will parse each of them separately, and store them in the stack_list
+    variable.
+
+    Args:
+      stacktrace: A string containing stacktrace.
+      chrome_build: A string containing the build type of the crash.
+    """
+    # If the passed in string is empty, the object does not represent anything.
+    if not stacktrace:
+      self.stack_list = None
+      return
+
+    # Reset the stack list, and assume we start from main thread.
+    self.stack_list = []
+    stack_priority = 0
+    reached_new_callstack = False
+    # Note that we do not need exact stack frame index, we only need relative
+    # position of a frame within a callstack. The reason for not extracting
+    # index from a line is that some stack frames do not have index.
+    stack_frame_index = 0
+    current_stack = CallStack(stack_priority)
+
+    for line in stacktrace:
+      # Check if current line is the start of new callstack

[Martin Barbella, 2014/08/06 21:12:25]

This comment isn't needed. The next line could stand on its own pretty well with
a minor name change. Also, remember to end comments with punctuation.

[jeun, 2014/08/06 23:36:25]

Done.

+      is_new_callstack = self.__CheckIfNewCallStack(line, chrome_build)

[Martin Barbella, 2014/08/06 21:12:24]

__CheckIfNewCallStack is a slightly awkward name. Maybe just something like
__IsStartOfNewStack.

[jeun, 2014/08/06 23:36:25]

Done.

+      if is_new_callstack:
+
+        # If this callstack is from main thread, update the boolean.
+        if not reached_new_callstack:
+          reached_new_callstack = True
+
+        # If this is from freed or prev-alloc, add the callstack we have
+        # to the list of callstacks, and increment the stack priority.
+        else:
+          stack_frame_index = 0
+          if current_stack.frame_list:
+            self.stack_list.append(current_stack)
+          stack_priority += 1

[Martin Barbella, 2014/08/06 21:12:25]

A while ago, I thought we discussed that free and allocation stacks should have
the same priority as one another. We didn't really have any data to go on at the
time, but we just guessed that they could both be equally useful.

Either way, having the priority set by the order of the stack alone might cause
some issues down the road. You might want to try to have a way to actually
detect what kind of stack you are working with and set priority based on that,
rather than the order you find it in. Hypothetically, if we realize later on
that the allocation stack provides much better information than the free stack,
changing how this works later could be problematic with this implementation.

[jeun, 2014/08/06 23:36:25]

i have fixed so that it would look at the marker within the stacktrace to infer
which priority it should get. I think it should work.

+          current_stack = CallStack(stack_priority)
+
+      # Parse function name, file path and line number from the line.
+      parsed_stack_frame_line = self.__ExtractFromStackFrame(

[Martin Barbella, 2014/08/06 21:12:24]

This is more of a general comment, but try to name functions in such a way that
you can tell what they do from the name alone where possible. In this case, it's
pretty clear from the comment above what this is doing. It would be even better
if the name was clear enough that the comment wasn't necessary.

[jeun, 2014/08/06 23:36:25]

Done.

+          line, chrome_build)
+
+      # If the line is malformed, ignore this line. Else, get the info.
+      if not parsed_stack_frame_line:
+        continue
+
+      (function, file_path, crashed_line_number) = parsed_stack_frame_line

[Martin Barbella, 2014/08/06 21:12:24]

Instead of dealing with multiple return values from this, maybe it would be
better to create a single helper to create the StackFrame object from the
required information.

[jeun, 2014/08/06 23:36:25]

Done.

+
+      # Normalize the file path so that it can be compared to repository path.
+      file_name = os.path.basename(file_path)
+      (component, file_path) = crash_utils.NormalizePathLinux(file_path)
+
+      # Currently supports only blink and chromium.
+      if component == 'blink' or component == 'chromium':
+        current_stack.Add(
+            StackFrame(stack_frame_index, component, file_name,
+                       function, file_path, crashed_line_number))
+
+      stack_frame_index += 1
+
+    # Add the current callstack only if there are frames in it.
+    if current_stack.frame_list:
+      self.stack_list.append(current_stack)
+
+  def __CheckIfNewCallStack(self, line, chrome_build):
+    """Check if this line is the start of the new callstack.
+
+    Since each builds have different format of stacktrace, the logic for
+    checking the line for all builds is handled in here.
+
+    Args:
+      line: Line to check for.
+      chrome_build: The name of the build.
+
+    Returns:
+      True if the line is the start of new callstack, False otherwise.
+    """
+    # Currently not supported.
+    if 'android' in chrome_build:
+      return False
+
+    if 'syzyasan' in chrome_build:
+      # In syzyasan build, new stack starts with 'crash stack:',
+      # 'freed stack:', etc.
+      callstack_start_pattern = re.compile(r'^.* stack:$')
+      return callstack_start_pattern.match(line)
+
+    # Stack frame pattern for asan, lsan, tsan, etc.
+    stack_frame_index_pattern = re.compile(r'#(\d+)')

[Martin Barbella, 2014/08/06 21:12:25]

Make sure that there is only whitespace before the #.

[jeun, 2014/08/06 23:36:25]

This line is deleted.

+
+    # Match the line to see if it is a valid stack frame.
+    stack_frame_index_match = stack_frame_index_pattern.match(line)
+
+    # If this line does not represent the crashing information, return False.
+    if not stack_frame_index_match:
+      return False
+
+    # Get index from the frame. If the index is 0, it means that new callstack
+    # starts in this line.
+    stack_frame_index = int(stack_frame_index_match.group(1))
+    return stack_frame_index == 0
+
+  def __ExtractFromStackFrame(self, line, chrome_build):
+    """Extracts information from a line in stacktrace.
+
+    Args:
+      line: A stacktrace string to extract data from.
+      chrome_build: A string containing the build type
+                    of this crash (e.g. linux_asan_chrome_mp).
+
+    Returns:
+      A triple containing the name of the function, the path of the file and
+      the crashed line number.
+    """
+    line_parts = line.split()
+    try:
+      # Tsan has different stack frame style from other builds.
+      if chrome_build.startswith('linux_tsan'):
+        # Filter out lines that are not stack frame.
+        stack_frame_index_pattern = re.compile(r'#(\d+)')

[Martin Barbella, 2014/08/06 21:12:25]

These three lines seem to be the same in both cases, so just move it to before
the if statement to avoid the duplication.

[jeun, 2014/08/06 23:36:25]

Done.

+        if not stack_frame_index_pattern.match(line_parts[0]):
+          return None
+
+        file_path_and_line = line_parts[-2]
+        function = ' '.join(line_parts[1:-2])
+
+      else:
+        # Filter out lines that are not stack frame.
+        stack_frame_index_pattern = re.compile(r'#(\d+)')
+        if not stack_frame_index_pattern.match(line_parts[0]):
+          return None
+
+        file_path_and_line = line_parts[-1]
+        function = ' '.join(line_parts[3:-1])
+
+      # Get file path and line info from the line.
+      file_path_and_line = file_path_and_line.split(':')
+      file_path = file_path_and_line[0]
+      crashed_line_number = int(file_path_and_line[1])
+      return (function, file_path, crashed_line_number)
+
+    # Return None if the line is malformed.
+    except IndexError:
+      return None
+    except ValueError:
+      return None
+
+  def __getitem__(self, index):
+    return self.stack_list[index]
+
+  def GetStackFromMainThread(self):

[Martin Barbella, 2014/08/06 21:12:25]

It makes more sense to call this the crash stack than the stack from the main
thread. There's a good chance that all of the stacks are from the same thread,
they just have different meaning. This came up in a few different places, so it
would be worth going back through to check for other uses like this.

[jeun, 2014/08/06 23:36:25]

Done.

+    return self.stack_list[0]
+
+
+class CallStack(object):
+  """Represents a call stack within a stacktrace.
+
+  It is a list of StackFrame object, and the stack represented by
+  this object is from main thread, freed thread or previously-allocated thread.
+  """
+
+  def __init__(self, stack_priority):
+    self.frame_list = []
+    self.priority = stack_priority
+
+  def Add(self, stacktrace_line):
+    self.frame_list.append(stacktrace_line)
+
+  def GetFirstN(self, n):

[Martin Barbella, 2014/08/06 21:12:24]

Consider renaming to something like GetTopNFrames. Depending on how this is
used, it will make what is going on a bit clearer.

[jeun, 2014/08/06 23:36:25]

Done.

+    return self.frame_list[:n]
+
+
+class StackFrame(object):
+  """Represents a frame in stacktrace.
+
+  Attributes:
+    index: An index of the stack frame.
+    component: A component this line represents, such as blink, chrome, etc.
+    file_name: The name of the file that crashed.
+    function: The function that caused the crash.
+    file_path: The path of the crashed file.
+    crashed_line_number: The line of the file that caused the crash.
+  """
+
+  def __init__(self, stack_frame_index, component, file_name,
+               function, file_path, crashed_line_number):
+    self.index = stack_frame_index
+    self.component = component
+    self.file_name = file_name
+    self.function = function
+    self.file_path = file_path
+    self.crashed_line_number = crashed_line_number