[lldb/crashlog] Refactor the CrashLogParser logic

[mib]

Summary

This patch changes the CrashLogParser class to be both the base class
and a Factory for the JSONCrashLogParser & TextCrashLogParser.

That should help remove some code duplication and ensure both class
have a parse method.

Signed-off-by: Med Ismail Bennani <[e-mail]>

[mib]

Add missing import

[kastiglione]

lldb/examples/python/crashlog.py

420–424

It looks like debugger, path, verbose are not propagated to the CrashLogParser subclass, is this an oversight or is there some magic? Secondly, args and kwargs used/needed?

lldb/examples/python/scripted_process/crashlog_scripted_process.py

12–14

Is this catch and re-raise of any use? Since you're refactoring, maybe delete it?

[JDevlieghere]

lldb/examples/python/crashlog.py

419

430

Can this stay in the JSONCrashLogParser?

629

lldb/examples/python/scripted_process/crashlog_scripted_process.py

13

Given that the module itself is called crashlog, I would keep the underscore to avoid potential naming collisions.

[mib]

lldb/examples/python/crashlog.py

430

I need it to get the exception dictionary for D131086 ...

[mib]

lldb/examples/python/crashlog.py

430

I took another look at this ... it seems that TextCrashLogParser doesn't set the data or exception attributes ... I'd need first to have both parsers produce the same exception dictionary before being able to use it. I'll do that on a follow-up patch.

[mib]

Address @JDevlieghere comments

[JDevlieghere]

lldb/examples/python/crashlog.py

419

This is not done (yet)

[mib]

Address @kastiglione comment

[kastiglione]

lldb/examples/python/crashlog.py

421–424

Is this comment needed (maybe it was in an earlier draft)? The body of the function isn't calling init, so it might be fine to not have it.

424

I have not seen the object().__new__(SomeClass) syntax. Why is it being used for TextCrashLogParser but not JSONCrashLogParser? Also, __new__ is a static method, could it be object.__new__(...)? Or is there a subtly that requires an object instance? Somewhat related, would it be better to say super().__new__(...)?

Also: one class construction explicitly forwards the arguments, the other does not. Is there a reason both aren't implicit (or both explicit)?

[kastiglione]

lldb/examples/python/crashlog.py

437

can this be an __init__?

[mib]

lldb/examples/python/crashlog.py

424

As you know, python class are implicitly derived from the object type, making object.__new__ and super().__new__ pretty much the same thing.

In this specific case, both the TextCrashLogParser and JSONCrashLogParser inherits from the CrashLogParser class, so JSONCrashLogParser will just inherits CrashLogParser.__new__ implementation if we don't override it, which creates a recursive loop.
That's why I'm calling the __new__ method specifying the class.

437

No unfortunately because at the __init__ is called, the type of the object is already set, but we actually want to fallback to the TextCrashLogParser class if this one fails.

__new__: takes arguments and returns an instance
__init__: takes instance and arguments (from __new__) and returns nothing

[JDevlieghere]

lldb/examples/python/crashlog.py

424

What's the advantage of this over this compared to a factory method? Seems like this could be:

def create(debugger, path, verbose)
    try:
            return JSONCrashLogParser(debugger, path, verbose)
        except CrashLogFormatException:
            return  TextCrashLogParser(debugger, path, verbose)

[mib]

lldb/examples/python/crashlog.py

424

If we make a factory, then users could still call __init__ on CrashLogParser and create a bogus object. With this approach, they're forced to instantiate a CrashLogParser like any another object.

[kastiglione]

lldb/examples/python/crashlog.py

424

CrashLogParser.__init__ could raise an exception. With intricacy of this approach, maybe it's better to use a factor method combined with an exception if the base class __init__ is called.

[JDevlieghere]

lldb/examples/python/crashlog.py

424

+1, or maybe abc provide a capability to achieve the same?

[mib]

lldb/examples/python/crashlog.py

424

IMHO, having to call an arbitrary-called method (create/make/...) to instantiate an object and having the __init__ raise an exception introduces more intricacies in the usage of this class, compared to what I'm doing.

I prefer to keep it this way since it's more natural / safe to use. If the implementation exercises some python internal features, that's fine because that shouldn't matter to the user.

[kastiglione]

lldb/examples/python/crashlog.py

424

Only after discussing it with you, and reading python docs, do I understand why this code is the way it is. Future editors, including us, could forget some details, which isn't great for maintainability.

You mention the user, are there external users of this class hierarchy? Or are these classes internal to crashlog.py? If the latter, then the simplified interface seems hypothetical. If there are external users, how many are they? I am trying to get a sense for what is gained by the avoiding a factory method.

[kastiglione]

lldb/examples/python/crashlog.py

424

here's an idea that may simplify things:

instead of embedding validation inside JSONCrashLogParser.__new__, have a static/class method for validation. Then, the base class can decide which subclass by calling the validation method. This means the subclasses don't need to override __new__. Ex:

class Base:
  def __new__(cls, i: int):
    if Sub1.is_valid(i):
      return object.__new__(Sub1)
    else:
      return object.__new__(Sub2)

  def __init__(self, i: int):
    print("Base", i)

class Sub1(Base):
  @staticmethod
  def is_valid(i: int):
    return i == 1234

  def __init__(self, i: int):
    super().__init__(i)
    print("Sub1", i)

class Sub2(Base):
  def __init__(self, i: int):
    super().__init__(i)
    print("Sub2", i)

[kastiglione]

was the chmod intentional?

[mib]

In D131085#3710597, @kastiglione wrote:

was the chmod intentional?

nop, thanks for catching that

[mib]

lldb/examples/python/crashlog.py

424

@JDevlieghere might be able to give more details on our users but crashlog.py could be imported in other python script, and from what he told me, we have some internal users that rely on it.

I don't have any strong opinion wrt your suggestion, I'll try it out and update the patch if that works well. Thanks @kastiglione !

[mib]

Implement @kastiglione suggestion

[kastiglione]

thanks for having patience with us reviewers, lgtm with one comment about python versions

lldb/examples/python/crashlog.py

427

the walrus operator (:=) is python 3.8 or newer, just want to make sure we don't expect this code to run in 3.6 or other earlier versions

[mib]

Remove walrus (:=) operator

[mib]

In D131085#3711104, @kastiglione wrote:

thanks for having patience with us reviewers, lgtm with one comment about python versions

Thanks for the review :) !

lldb/examples/python/crashlog.py

 #
 # On MacOSX csh, tcsh:
 #   ( setenv PYTHONPATH /path/to/LLDB.framework/Resources/Python ; ./crashlog.py ~/Library/Logs/DiagnosticReports/a.crash )
 #
 # On MacOSX sh, bash:
 #   PYTHONPATH=/path/to/LLDB.framework/Resources/Python ./crashlog.py ~/Library/Logs/DiagnosticReports/a.crash
 #----------------------------------------------------------------------
 
+import abc
 import concurrent.futures
 import contextlib
 import datetime
 import json
 import optparse
 import os
 import platform
 import plistlib

 
 
 class CrashLogParseException(Exception):
     pass
 
 class InteractiveCrashLogException(Exception):
     pass
 
-class CrashLogParser:
-    def parse(self, debugger, path, verbose):
+class CrashLogParser(object):
+    "CrashLog parser base class and factory."
+    def __new__(cls, debugger, path, verbose, *args, **kwargs):
         try:
-            return JSONCrashLogParser(debugger, path, verbose).parse()
+            return super(CrashLogParser, cls).__new__(JSONCrashLogParser, *args, **kwargs)
         except CrashLogFormatException:
-            return TextCrashLogParser(debugger, path, verbose).parse()
+            return super(CrashLogParser, cls).__new__(TextCrashLogParser, *args, **kwargs)
 
-
-class JSONCrashLogParser:
     def __init__(self, debugger, path, verbose):
         self.path = os.path.expanduser(path)
         self.verbose = verbose
         self.crashlog = CrashLog(debugger, self.path, self.verbose)
 
+    @abc.abstractmethod
+    def parse(self):
+        pass
+
+
+class JSONCrashLogParser(CrashLogParser):
+    def __init__(self, debugger, path, verbose):
+        super().__init__(debugger, path, verbose)
+
     def parse_json(self, buffer):
         try:
             return json.loads(buffer)
         except:
             # The first line can contain meta data. Try stripping it and try
             # again.
             head, _, tail = buffer.partition('\n')
             return json.loads(tail)
class CrashLogParseMode:
     NORMAL = 0
     THREAD = 1
     IMAGES = 2
     THREGS = 3
     SYSTEM = 4
     INSTRS = 5
 
 
-class TextCrashLogParser:
+class TextCrashLogParser(CrashLogParser):
     parent_process_regex = re.compile('^Parent Process:\s*(.*)\[(\d+)\]')
     thread_state_regex = re.compile('^Thread ([0-9]+) crashed with')
     thread_instrs_regex = re.compile('^Thread ([0-9]+) instruction stream')
     thread_regex = re.compile('^Thread ([0-9]+)([^:]*):(.*)')
     app_backtrace_regex = re.compile('^Application Specific Backtrace ([0-9]+)([^:]*):(.*)')
     version = r'(\(.+\)|(arm|x86_)[0-9a-z]+)\s+'
     frame_regex = re.compile(r'^([0-9]+)' r'\s+'                # id
                              r'(.+?)' r'\s+'                    # img_name
                              r'(' +version+ r')?'               # img_version
                              r'(0x[0-9a-fA-F]{7,})'             # addr (7 chars or more)
                              r' +(.*)'                          # offs
                             )
     null_frame_regex = re.compile(r'^([0-9]+)\s+\?\?\?\s+(0{7}0+) +(.*)')
     image_regex_uuid = re.compile(r'(0x[0-9a-fA-F]+)'            # img_lo
                                   r'\s+' '-' r'\s+'              #   -
                                   r'(0x[0-9a-fA-F]+)'     r'\s+' # img_hi
                                   r'[+]?(.+?)'            r'\s+' # img_name
                                   r'(' +version+ ')?'            # img_version
                                   r'(<([-0-9a-fA-F]+)>\s+)?'     # img_uuid
                                   r'(/.*)'                       # img_path
                                  )
 
 
     def __init__(self, debugger, path, verbose):
-        self.path = os.path.expanduser(path)
-        self.verbose = verbose
+        super().__init__(debugger, path, verbose)
         self.thread = None
         self.app_specific_backtrace = False
-        self.crashlog = CrashLog(debugger, self.path, self.verbose)
         self.parse_mode = CrashLogParseMode.NORMAL
         self.parsers = {
             CrashLogParseMode.NORMAL : self.parse_normal,
             CrashLogParseMode.THREAD : self.parse_thread,
             CrashLogParseMode.IMAGES : self.parse_images,
             CrashLogParseMode.THREGS : self.parse_thread_registers,
             CrashLogParseMode.SYSTEM : self.parse_system,
             CrashLogParseMode.INSTRS : self.parse_instructions,
if crash_log.errors:
         for error in crash_log.errors:
             print(error)
 
 def load_crashlog_in_scripted_process(debugger, crash_log_file, options, result):
     crashlog_path = os.path.expanduser(crash_log_file)
     if not os.path.exists(crashlog_path):
         raise InteractiveCrashLogException("crashlog file %s does not exist" % crashlog_path)
 
-    crashlog = CrashLogParser().parse(debugger, crashlog_path, False)
+    crashlog = CrashLogParser(debugger, crashlog_path, False).parse()
 
     target = lldb.SBTarget()
     # 1. Try to use the user-provided target
     if options.target_path:
         target = debugger.CreateTarget(options.target_path)
         if not target:
             raise InteractiveCrashLogException("couldn't create target provided by the user (%s)" % options.target_path)
 
if args:
         for crash_log_file in args:
             if should_run_in_interactive_mode(options, ci):
                 try:
                     load_crashlog_in_scripted_process(debugger, crash_log_file,
                                                       options, result)
                 except InteractiveCrashLogException as e:
                     result.SetError(str(e))
             else:
-                crash_log = CrashLogParser().parse(debugger, crash_log_file, options.verbose)
+                crash_log = CrashLogParser(debugger, crash_log_file, options.verbose).parse()
                 SymbolicateCrashLog(crash_log, options)
 
 if __name__ == '__main__':
     # Create a new debugger instance
     debugger = lldb.SBDebugger.Create()
     result = lldb.SBCommandReturnObject()
     SymbolicateCrashLogs(debugger, sys.argv[1:], result)
     lldb.SBDebugger.Destroy(debugger)
 
 def __lldb_init_module(debugger, internal_dict):
     debugger.HandleCommand(
         'command script add -c lldb.macosx.crashlog.Symbolicate crashlog')
     debugger.HandleCommand(
         'command script add -f lldb.macosx.crashlog.save_crashlog save_crashlog')
     print('"crashlog" and "save_crashlog" commands have been installed, use '
           'the "--help" options on these commands for detailed help.')

lldb/examples/python/scripted_process/crashlog_scripted_process.py

 import os,json,struct,signal,uuid
 
 from typing import Any, Dict
 
 import lldb
 from lldb.plugins.scripted_process import ScriptedProcess
 from lldb.plugins.scripted_process import ScriptedThread
 
 from lldb.macosx.crashlog import CrashLog,CrashLogParser
 
 class CrashLogScriptedProcess(ScriptedProcess):
     def parse_crashlog(self):
-        try:
-            crash_log = CrashLogParser().parse(self.dbg, self.crashlog_path, False)
-        except Exception as e:
-            raise e
+        crashlog_parser = CrashLogParser(self.dbg, self.crashlog_path, False)
+        crash_log = crashlog_parser.parse()
 
         self.pid = crash_log.process_id
         self.addr_mask = crash_log.addr_mask
         self.crashed_thread_idx = crash_log.crashed_thread_idx
         self.loaded_images = []
 
         def load_images(self, images):
             #TODO: Add to self.loaded_images and load images in lldb