[Lldb-commits] [PATCH] D132191: Treat a UUID of all zeros consistently to mean "Invalid UUID"

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Remember, all this patch does is make a universal rule that "All zero UUID's are placeholders and not meant to be matched against."  The only platforms/ObjectFile readers that weren't already following that rule were Darwin - where this was definitely just historical accident - and ObjectFileELF.  Then there were a few places in generic code where we seem to have tossed a coin for which behavior to use.  Windows & Breakpad were already calling the "Optional" version of the set functions.  So this is only a change in behavior for ELF.



================
Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:1392
       image_infos[i].SetName((const char *)name_data);
-      UUID uuid = UUID::fromOptionalData(extractor.GetData(&offset, 16), 16);
+      UUID uuid = UUID::fromData(extractor.GetData(&offset, 16), 16);
       image_infos[i].SetUUID(uuid);
----------------
clayborg wrote:
> Have you checked if the kernel ever just specifies all zeroes here? It it does, this will change things right?
On Darwin, UUID's of all zeros always means "I had to put in a UUID, but I don't actually want you to use it".  I don't know whether the Kernel currently produces this or not - it's currently used for the stub binaries that Playgrounds uses.


================
Comment at: lldb/source/Plugins/Process/minidump/MinidumpParser.cpp:70-72
       if (pdb70_uuid->Age != 0)
-        return UUID::fromOptionalData(pdb70_uuid, sizeof(*pdb70_uuid));
-      return UUID::fromOptionalData(&pdb70_uuid->Uuid,
+        return UUID::fromData(pdb70_uuid, sizeof(*pdb70_uuid));
+      return UUID::fromData(&pdb70_uuid->Uuid,
----------------
clayborg wrote:
> Fix the UUID UUID::fromCvRecord(UUID::CvRecordPdb70) function and call that function from here.
Note that fromCvRecord already used the fromOptional version, so its behavior doesn't change with this patch.  I don't think anything else needs to be done here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132191