[Lldb-commits] [PATCH] D146765: [lldb/crashlog] Load inlined symbol into interactive crashlog

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

Looks good! A few minor comments.



================
Comment at: lldb/examples/python/crashlog.py:628-629
+            description = ""
+            # Since images are parsed after threads, we need to build a
+            # map for every image with a list of all the symbols and addresses
+            if frame_img_name and frame_addr and frame_symbol:
----------------
I think this comment better describes `parse_thread` rather than `parse_asi_backtrace` no?


================
Comment at: lldb/examples/python/crashlog.py:634-635
+                    description += " + " + frame_offset
+                for image in self.images:
+                    if image.identifier == frame_img_name:
+                        image.symbols[frame_symbol] = {
----------------
Does it make sense for `self.images` to be a dictionary instead of a list? Something that maps `image_name -> image_info`. That way you could do something like `self.images[frame_img_name].symbols[frame_symbol] = { ... }` instead of iterating over every image (with appropriate error checking in the event that frame_img_name isn't in self.images yet).


================
Comment at: lldb/examples/python/crashlog.py:639
+                            "type": "code",
+                            "address": int(frame_addr, 0) - int(frame_offset)
+                        }
----------------
if `frame_offset` is `None`, you're doing `int(None)` which will give you a `TypeError`. Above you can do something like `frame_offset_value = 0` and in the `if frame_offset:` block you can do `frame_offset_value = int(frame_offset)`.


================
Comment at: lldb/examples/python/crashlog.py:708
+                        r'))(?: '    # capture group garbage
+                        r'\('        # source infor capture group
+                        r'([^\:]+)'  # file name
----------------
typo: `infor` -> `info`


================
Comment at: lldb/examples/python/crashlog.py:711
+                        r'\:(\d+)'   # line number
+                        r'(?:\:'     # capture group garbage
+                        r'(\d+)'     # column number
----------------
What does `garbage` mean in this case? I assume it's boilerplate of some kind.


================
Comment at: lldb/examples/python/crashlog.py:936
+                description += " + " + frame_offset
+            if not frame_img_name in self.symbols:
+                self.symbols[frame_img_name] = list()
----------------



================
Comment at: lldb/examples/python/crashlog.py:940
+                "name": frame_symbol,
+                "address": int(frame_addr, 0) - int(frame_offset)
+            })
----------------
Same situation here as above, `frame_offset` might be `None` and then you'll crash with a `TypeError`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146765/new/

https://reviews.llvm.org/D146765