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

[Pavel Labath via Phabricator via lldb-commits]

labath added a reviewer: markmentovai.
labath marked 3 inline comments as done.
labath added inline comments.


================
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)
----------------
markmentovai wrote:
> 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.
Thanks. I've implemented the logic you suggested and fixed byte-swapping issues when parsing the module id.

Note I still have to special-case windows to strip the "age" field from the module_id in order for our UUID to match the ones we normally get on mac. (We do the same thing when opening minidump files: <https://github.com/llvm-mirror/lldb/blob/master/source/Plugins/Process/minidump/MinidumpParser.cpp#L88>). 


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

https://reviews.llvm.org/D55214