Page MenuHomePhabricator

[lldb][NFC] Refactor getUUID functionality
ClosedPublic

Authored by zequanwu on Oct 28 2020, 11:24 AM.

Diff Detail

Event Timeline

zequanwu created this revision.Oct 28 2020, 11:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 28 2020, 11:24 AM
zequanwu requested review of this revision.Oct 28 2020, 11:24 AM
zequanwu updated this revision to Diff 301378.Oct 28 2020, 12:16 PM

include correct header

Thanks for cleaning this up. A couple of comments inline.

lldb/include/lldb/Utility/UUID.h
41

I'd name this like fromCvRecord or something. Also, replace the pointer by a reference, please.

lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
76

This is the only place which passes false, right?
If that's true, I think it would be better to drop this argument, and do something special here. That would give as an opportunity to call out the fact that breakpad (used to) put elf build-ids into this field. This is a sufficiently weird thing to merit drawing extra attention to it. Maybe something like:

if (isBinFormatELF()) {
  // Older versions of breakpad used to write the (first 16 bytes of) ELF build into this field.
  return UUID::fromOptionalData(pdb70_uuid->Uuid, sizeof(pdb70_uuid->Uuid))
}
return UUID::fromCvRecord(*pdb70_uuid);
lldb/source/Utility/UUID.cpp
38

You could just take the argument by value, and then byte-swap it in-place.

zequanwu updated this revision to Diff 301694.Oct 29 2020, 11:17 AM
zequanwu marked 3 inline comments as done.

Address comments.

labath accepted this revision.Oct 30 2020, 8:09 AM

lgtm, modulo the comment.

lldb/source/Plugins/Process/minidump/MinidumpParser.cpp
75

Once the reference is gone, you also don't need to make a copy here.

lldb/source/Utility/UUID.cpp
38

My previous comments were a bit confusing, but taking the argument by a mutable reference, was not what I intended. :(
I first wanted to use a const reference as that's better than a pointer, but then I realized that you're also making a copy, so it would be better if we just had the compiler do that. So you can just drop the reference now.

Sorry.

This revision is now accepted and ready to land.Oct 30 2020, 8:09 AM
zequanwu updated this revision to Diff 301957.Oct 30 2020, 10:44 AM
zequanwu marked 2 inline comments as done.

Address comments.

This revision was landed with ongoing or failed builds.Oct 30 2020, 10:45 AM
This revision was automatically updated to reflect the committed changes.