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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
