[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..c1f139e6b74dd311e65b856b0af29e5f4bc1038d
--- /dev/null
+++ b/tools/findit/stacktrace.py
@@ -0,0 +1,173 @@
+# 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 findit_for_crash_utils as findit_utils

[stgao, 2014/07/31 01:52:24]

How about adding a file crash_utils.py? And include the needed
findit_utils.NormalizePathLinux in it for the moment?

[jeun, 2014/07/31 18:41:15]

Done.

+
+
+class Stacktrace(object):

[stgao, 2014/07/31 01:52:24]

Rename to |StacktraceInfo|? Because it contains multiple stack traces.

[jeun, 2014/07/31 18:41:14]

I named it stacktrace because this object exists one for release build
stacktrace and one for debug build stacktrace, so it would be natural to call it
just stacktrace. Also I'm not sure if adding Info would imply that this object
is a list of callstacks.

[stgao, 2014/07/31 19:48:58]

Sound good.
In that case, the comment "Contains release build stacktrace and debug build
stacktrace" might need updating.

+  """Represents Stacktrace object.
+
+  Contains release build stacktrace and debug build stacktrace.
+  """
+
+  def __init__(self, stacktrace, chrome_build):
+    self.stack_list = []
+    self.ParseStacktrace(stacktrace, chrome_build)
+
+  def ParseStacktrace(self, stacktrace, chrome_build):
+    """Parses stacktrace and returns normalized stacktrace.

[stgao, 2014/07/31 01:52:24]

We do not return anything within this function actually.

[jeun, 2014/07/31 18:41:15]

Done.

+
+    If there are multiple stack frames within the stacktrace,

[stgao, 2014/07/31 01:52:24]

What's the 'stack frame' here? Does it refer to a call stack of a thread?

[jeun, 2014/07/31 18:41:15]

changed the description 

+    it will parse each frame separately.
+
+    Args:
+      stacktrace: a string containing stacktrace
+      chrome_build: a string containing the job type of the crash
+
+    Returns:
+      A stack dictionary, where key 'release' containing the result of parsing
+      release build stacktrace and 'debug' containing the result from debug
+      build stacktrace. Parsed result from each stacktrace contains a list
+      of parsed result for each stack within the stacktrace, where a
+      parsed result is a list of stacktraceline objects.
+    """
+    # If the passed in string is empty, the object does not represent anything
+    if not stacktrace:
+      self.stack_list = None
+      return
+
+    stack_frame_index_pattern = re.compile(r'#\d+')
+
+    # Reset the stack list, and assume we start from main thread

[stgao, 2014/07/31 01:52:24]

# Comments end with a period like this.

Please also check all comments in all files.
This style issue appears in many places.

[jeun, 2014/07/31 18:41:15]

Done.

+    self.stack_list = []
+    stack_priority = 0
+    seen_zero = False
+    current_stack = CallStack(stack_priority)
+
+    for line in stacktrace:
+
+      # If this line does not represent the crashing information, continue
+      if not stack_frame_index_pattern.match(line):
+        continue
+
+      # Get the frame index from the line
+      parts = line.split()
+      stack_frame_index = int(parts[0][1:])

[stgao, 2014/07/31 01:52:25]

Use regex to get the stack_frame_index instead of a splitting here.

stack_frame_index_pattern = re.compile(r'#(\d+)')
match = stack_frame_index_pattern.match(line)
if not match:
  continue

stack_frame_index = match.group(1)

[jeun, 2014/07/31 18:41:15]

Done.

+
+      # If it is 0, we need to check if this is from freed or prev-alloc, or
+      # is from main thead
+      if stack_frame_index == 0:
+
+        # If this frame is from main thread

[stgao, 2014/07/31 01:52:24]

frame?

[jeun, 2014/07/31 18:41:15]

Done.

+        if not seen_zero:

[stgao, 2014/07/31 01:52:24]

Could |seen_zero| be renamed to |seen_main_thread|?

[jeun, 2014/07/31 18:41:15]

Done.

+          seen_zero = 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:
+          self.stack_list.append(current_stack)
+          stack_priority += 1
+          current_stack = CallStack(stack_priority)
+
+      # Parse function name, file path and line number from the line
+      parsed_stack_frame_line = self.ExtractFromStacktraceLine(
+          parts, 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) = parsed_stack_frame_line
+
+      # Normalize the file path so that it can be compared to repository path
+      file_name = os.path.basename(file_path)
+      (component, file_path) = findit_utils.NormalizePathLinux(file_path)
+
+      # Currently supports only blink and chromium
+      if component == 'blink' or component == 'chromium':
+        current_stack.Add(
+            StacktraceLine(stack_frame_index, component, file_name,
+                           function, file_path, crashed_line))
+
+    self.stack_list.append(current_stack)

[stgao, 2014/07/31 01:52:24]

I think different builds (asan, lsan, tsan, msan) will have different format of
stacktrace output.

Would you mind making this function as a separate class
SanitizerStackTraceParser?

We could also add an interface like below and make SanitizerStackTraceParser
implement it:

class StackTraceParser(object):

  def support(self, chrome_build_type):
    # Return True if the build type is supported by this parser, otherwise
return False.
    raise NotImplemented()

  def parse(self, stack_trace_string):
    # Parse the stack trace, and return an instance of StackTraceInfo.
    raise NotImplemented()


The design pattern here is chain of responsibility.
It is extendable if we want to support more types of stack traces.

[jeun, 2014/07/31 18:41:14]

I agree that something needs to be done about this file, but I think it might be
a bit overkill. In order to handle multiple builds, we only need different rules
for extracting file path/line/function from a line. So maybe create bunch of
classes with rules on how to parse a line for certain builds, instead of classes
to parse the whole stacktrace?

+
+  def ExtractFromStacktraceLine(self, parts, chrome_build):
+    """Extracts information from stacktrace.

[stgao, 2014/07/31 01:52:25]

stacktrace seems misused here. Should be stack frame, right?

[jeun, 2014/07/31 18:41:15]

Done.

+
+    Args:
+      parts: a list, stacktrace.split()
+      chrome_build: a string containing the job 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 line of crash
+    """
+    try:
+      # tsan has different stack frame style from other builds
+      if chrome_build.startswith('linux_tsan'):
+        crash_component = parts[-2]

[stgao, 2014/07/31 01:52:24]

The way we guess |crash_component| here is hacky.
Would you mind adding a TODO for improvement later by integrating with the DEPS
file?

[jeun, 2014/07/31 18:41:15]

This is just a totally mis named variable. changed the name.

+        function = ' '.join(parts[1:-2])
+
+      else:
+        crash_component = parts[-1]
+        function = ' '.join(parts[3:-1])
+
+      # Get file path and line info from the line
+      file_and_line = crash_component.split(':')
+      file_path = file_and_line[0]
+      crashed_line = int(file_and_line[1])
+      return (function, file_path, crashed_line)
+
+    # 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):
+    return self.stack_list[0]
+
+
+class CallStack(object):
+  """Represents a call stack within a stacktrace.
+
+  It is a list of stacktraceline object, and the stack represented by
+  this object is from main thread, freed thread or previously-allocated thread.
+  """
+
+  def __init__(self, stack_priority):
+    self.line_list = []

[stgao, 2014/07/31 01:52:25]

|line_list| -> |frames| or |frame_list|?

[jeun, 2014/07/31 18:41:15]

Done.

+    self.priority = stack_priority
+
+  def Add(self, stacktrace_line):
+    self.line_list.append(stacktrace_line)
+
+  def GetFirstN(self, n):

[stgao, 2014/07/31 01:52:24]

check out of boundary?

[jeun, 2014/07/31 18:41:15]

Python automatically checks for boundary when using slicing.

+    return self.line_list[:n]
+
+
+class StacktraceLine(object):
+  """Represents a line in stacktrace.

[stgao, 2014/07/31 01:52:25]

We refer this as stack frames. Maybe renaming it to StackFrame and
|stack_frame_index| to |index|?

[jeun, 2014/07/31 18:41:15]

Done.

+
+  Attributes:
+    stack_frame_index: index in the stacktrace
+    component: a component this line represents, such as blink, chrome, etc.
+    file_name: name of the file that crashed

[stgao, 2014/07/31 01:52:24]

|file_name| could be derived from |file_path|. Could add a property function
which returns the file_name from the file_path.

[jeun, 2014/07/31 18:41:15]

Same as the comment in component_dictionary file.

+    function: function that caused the crash
+    file_path: path of the crashed file
+    crashed_line: line of the file that caused the crash

[stgao, 2014/07/31 01:52:25]

Will |line_number| be more clear?

[jeun, 2014/07/31 18:41:15]

changed to crashed_line_number

+  """
+
+  def __init__(self, stack_frame_index, component, file_name,
+               function, file_path, crashed_line):
+    self.stack_frame_index = stack_frame_index
+    self.component = component
+    self.file_name = file_name
+    self.function = function
+    self.file_path = file_path
+    self.crashed_line = crashed_line