[Lldb-commits] [PATCH] D57037: BreakpadRecords: Address post-commit feedback

[Pavel Labath via Phabricator via lldb-commits]

labath marked 2 inline comments as done.
labath added inline comments.


================
Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:67
+/// to the endian-specific integer N. Return true on success.
+template <typename T> static bool consume_integer(llvm::StringRef &str, T &N) {
+  llvm::StringRef chunk = str.take_front(hex_digits<T>());
----------------
clayborg wrote:
> rename to "consume_hex_integer" or an extra parameter for the base instead of hard coding to 16?
I'll rename the function before submitting.


================
Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:111-112
   // match the native uuid format of these platforms.
-  return UUID::fromData(&data, os == llvm::Triple::Win32 ? 20 : 16);
+  return UUID::fromData(&data, os == llvm::Triple::Win32 ? sizeof(data)
+                                                         : sizeof(data.uuid));
 }
----------------
clayborg wrote:
> For Apple platforms Breakpad actually incorrectly byte swaps the first 32 bits and the next two 16 bit values. I have a patch for this, but since this is moving, it would be great to get that fix in here. Also, many linux breakpad file have the UUID field present but set to all zeroes which is bad as UUID parsing code will cause multiple modules to claim a UUID of all zeroes is valid and causes all modules with such a UUID to just use the first one it finds. The code I have in my unsubmitted patch is:
> 
> ```
>       if (pdb70_uuid->Age == 0) {
>         bool all_zeroes = true;
>         for (size_t i=0; all_zeroes && i<sizeof(pdb70_uuid->Uuid); ++i)
>           all_zeroes = pdb70_uuid->Uuid[i] == 0;
>         // Many times UUIDs are not filled in at all, so avoid claiming that
>         // all such libraries have a valid UUID that is all zeroes.
>         if (all_zeroes)
>           return UUID();
>         if (arch.GetTriple().getVendor() == llvm::Triple::Apple) {
>           // Breakpad incorrectly byte swaps the first 32 bit and next 2 16 bit
>           // values in the UUID field. Undo this so we can match things up
>           // with our symbol files
>           uint8_t apple_uuid[16];
>           // Byte swap the first 32 bits
>           apple_uuid[0] = pdb70_uuid->Uuid[3];
>           apple_uuid[1] = pdb70_uuid->Uuid[2];
>           apple_uuid[2] = pdb70_uuid->Uuid[1];
>           apple_uuid[3] = pdb70_uuid->Uuid[0];
>           // Byte swap the next 16 bit value
>           apple_uuid[4] = pdb70_uuid->Uuid[5];
>           apple_uuid[5] = pdb70_uuid->Uuid[4];
>           // Byte swap the next 16 bit value
>           apple_uuid[6] = pdb70_uuid->Uuid[7];
>           apple_uuid[7] = pdb70_uuid->Uuid[6];
>           for (size_t i=8; i<sizeof(pdb70_uuid->Uuid); ++i)
>             apple_uuid[i] = pdb70_uuid->Uuid[i];
>           return UUID::fromData(apple_uuid, sizeof(apple_uuid));
>         } else
>           return UUID::fromData(pdb70_uuid->Uuid, sizeof(pdb70_uuid->Uuid));
>       } else
>         return UUID::fromData(pdb70_uuid, sizeof(*pdb70_uuid));
> ```
The byte swapping is already handled in this code (that's why it's this complicated). My impression was that the swapping is not apple-specific, but rather depends where you read the UUID from (MODULE record has it swapped, INFO record doesn't). Though that may depend on how we chose to interpret the raw bytes in other UUID sources (object files, pdb files, ...).

As for the all-zero case, that should be easily fixable, by changing fromData to fromOptionalData, but I'm surprised that this is necessary, as I was under the impression that breakpad invent's its own UUIDs in case the original object file doesn't have them. (This would actually be the better case, as otherwise we'd have to figure out how to tell the fictional and non-fictional uuids apart.) @lemo: Can you shed any light on this?


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

https://reviews.llvm.org/D57037