[Lldb-commits] [PATCH] D59433: Fix UUID decoding from minidump files.

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

Looks good to me. I have some comments inline and below, but none of them is really substantial.

In D59433#1431602 <https://reviews.llvm.org/D59433#1431602>, @clayborg wrote:

> In D59433#1431488 <https://reviews.llvm.org/D59433#1431488>, @zturner wrote:
>
> > "MinidumpNew" is a little bit vague.  What's "new" about it?  Is there a way we could come up with a better name?
>
>
> This was an existing file that I just added new tests to, and it seemed to be where most of the minidump tests were.


The name is a relict from when we were implementing our own minidump reader. In that context, the existing implementation which relied on the windows APIs was the "old" thing, and the one which read the files directly was "new". But this distinction doesn't really make sense anymore, so maybe we could start getting rid of it by putting the new tests into a fresh file (`TestMinidumpUUIDs.py` ?).

> 
> 
>> As an aside, since there's no interactivity in these tests, they seem like a good candidate for lit/Minidump, with 1 file for each test.  Something like:
>> 
>>   ; zero-uuid-modules.test
>>   ; RUN: lldb -core %S/Inputs/linux-arm-zero-uuids.dmp -S %p | FileCheck %s
>>   
>>   target modules list
>>   
>>   ; CHECK: [  0] 00000000-0000-0000-0000-000000000000-00000000                    /tmp/a
>>   ; CHECK: [  1] 00000000-0000-0000-0000-000000000000-00000000                    /tmp/b
>> 
>> 
>> Let's see what Pavel thinks though.

I think turning these into lit tests would be reasonable, but I also don't have a problem with them staying like they are.



================
Comment at: packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py:533
+        self.assertEqual(2, len(modules))
+        self.verify_module(modules[0], "/tmp/a", None)
+        self.verify_module(modules[1], "/tmp/b", None)
----------------
Am I right in thinking that if I have an object file called `/tmp/a` on my system, then lldb will pick it up (and it's build-id), causing this test to fail ? If that's the case then it would be better to use some path which is less likely to exist.


================
Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:175-183
+
+    // Many times UUIDs are all zeroes. This can cause more than one module
+    // to claim it has a valid UUID of all zeroes and causes the files to all
+    // merge into the first module that claims this valid zero UUID.
+    bool all_zeroes = true;
+    for (size_t i = 0; all_zeroes && i < sizeof(pdb70_uuid->Uuid); ++i)
+      all_zeroes = pdb70_uuid->Uuid[i] == 0;
----------------
Instead of all of this, I believe you should be able to just replace the `UUID::fromData` with `UUID::fromOptionalData` in the calls below.


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

https://reviews.llvm.org/D59433