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

[Med Ismail Bennani via Phabricator via lldb-commits]

mib marked 7 inline comments as done.
mib added inline 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:
----------------
bulbazord wrote:
> I think this comment better describes `parse_thread` rather than `parse_asi_backtrace` no?
+1, that's an oversight.


================
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] = {
----------------
bulbazord wrote:
> 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).
I thought about that but the problem is that `JSONCrashLogParser` uses image index and `TextCrashLogParser` uses image name as dictionary keys. It would be pretty convoluted to make the dictionary that is the same for both parser so I'd rather keep a list here.


================
Comment at: lldb/examples/python/crashlog.py:639
+                            "type": "code",
+                            "address": int(frame_addr, 0) - int(frame_offset)
+                        }
----------------
bulbazord wrote:
> 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)`.
Good point!


================
Comment at: lldb/examples/python/crashlog.py:711
+                        r'\:(\d+)'   # line number
+                        r'(?:\:'     # capture group garbage
+                        r'(\d+)'     # column number
----------------
bulbazord wrote:
> What does `garbage` mean in this case? I assume it's boilerplate of some kind.
When I broke down the regex, I really to isolate the capture groups that we cared the most about.
By "garbage" I mean text that are required to parse the whole thing but that isn't captured.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146765