[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

[Mark Mentovai via Phabricator via lldb-commits]

markmentovai added a comment.

Very excited to see this work beginning!



================
Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:66-84
+  if (os == llvm::Triple::Win32) {
+    // In binary form, the module id should have 20 bytes: 16 bytes for UUID,
+    // and 4 bytes for the "age". However, in the textual format, the 4 bytes of
+    // age are printed via %x, which can lead to shorter strings. So, we pad the
+    // string with zeroes after the 16 bytes, to obtain a string of appropriate
+    // size.
+    if (token.size() < 33 || token.size() > 40)
----------------
labath wrote:
> @lemo: Does this part make sense? It seems that on linux the breakpad files have the `INFO CODE_ID` section, which contains the UUID without the funny trailing zero. So I could try fetching the UUID from there instead, but only on linux, as that section is not present mac (and on windows it contains something completely different). Right now I compute the UUID on linux by chopping off the trailing zero (as I have to do that anyway for mac), but I could do something different is there's any advantage to that.
INFO CODE_ID, if present, is a better thing to use than what you find in MODULE, except on Windows, where it’s absolutely the wrong thing to use but MODULE is fine.

So, suggested logic:

  if has_code_id and not is_win:
    id = code_id
  else:
    id = module_id

Aside from special-casing Windows against using INFO CODE_ID, I don’t think you should hard-code any OS checks here. There’s no reason Mac dump_syms couldn’t emit INFO CODE_ID, even though it doesn’t currently.

(In fact, you don’t even need to special-case for Windows. You could just detect the presence of a filename token after the ID in INFO CODE_ID. As your test data shows, Windows dump_syms always puts the module filename here, as in “INFO CODE_ID 5C01672A4000 a.exe”, but other dump_syms will only have the uncorrupted debug ID.


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

https://reviews.llvm.org/D55214