labath added a comment.
Thanks for cleaning this up. A couple of comments inline.
================
Comment at: lldb/include/lldb/Utility/UUID.h:41
+ /// Create a UUID from CvRecordPdb70.
+ static UUID fromData(const CvRecordPdb70 *debug_info,
+ bool swapByteOrder = true);
----------------
I'd name this like `fromCvRecord` or something. Also, replace the pointer by a
reference, please.
================
Comment at: lldb/source/Plugins/Process/minidump/MinidumpParser.cpp:70
+ return UUID::fromData(pdb70_uuid,
+ !GetArchitecture().GetTriple().isOSBinFormatELF());
} else if (cv_signature == CvSignature::ElfBuildId)
----------------
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);
```
================
Comment at: lldb/source/Utility/UUID.cpp:38
+UUID UUID::fromData(const UUID::CvRecordPdb70 *debug_info, bool swapByteOrder)
{
+ UUID::CvRecordPdb70 swapped;
----------------
You could just take the argument by value, and then byte-swap it in-place.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90325/new/
https://reviews.llvm.org/D90325
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
