[Lldb-commits] [PATCH] D60001: Allow partial UUID matching in Minidump core file plug-in

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:394
+    if (error.Fail()) {
+      GetTarget().GetImages().Remove(module_sp);
+      module_sp.reset();
----------------
clayborg wrote:
> labath wrote:
> > Under what circumstances can `GetSharedModule` fail, and still produce a valid module_sp ?
> Sometimes you can create a module with a specification, but it won't have a valid object file is my understanding.
Ok, but if that's the what the function is supposed to do in that case, then why should it return an error?

To me, this looks like a bug in that function (I don't see any justification for GetSharedModule to modify the module list of a target, but still return an error), and it should be fixed there (and not worked around here). Or, if you don't know how to trigger this condition, we can just drop this code here.


================
Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:397
+    }
+    if (!module_sp) {
+      // Try and find a module without specifying the UUID and only looking for
----------------
clayborg wrote:
> labath wrote:
> > I am wondering if there's any way to narrow down the scope of this so we don't do this second check for every module out there. If I understood the description correctly, it should be sufficient to do this for linux minidumps with PDB70 uuid records. Since PDBs aren't a thing on linux, and later breakpad versions used used ElfBuildId records, this should be a pretty exact match for the situation we want to capture. WDYT?
> > 
> > (Getting the platform of the minidump should be easy, for the UUID record type, we might need a small refactor of the MinidumpParser class.)
> We really do want this. Why? Because many times with minidumps you have to go out and find the symbol files in some alternate location, put them in a directory, then we want lldb to find them automatically. If you have a module spec with a full path and no UUID, the path will need to match. If you just specify a basename with no UUID, it allows us to use that file. Also with our UUID story being a bit weak with ELF 20 bytes build IDs versus 4 bytes GNU build IDs,. we might need to pull some tricks and strip out a UUID from a binary so we can load it in LLDB. So this basename only search allows us to use a symbol file with no UUID or if the minidump didn't contain a UUID and somehow we knew where the right symbol file was. So this does catch my case, but will also find any files in the exec search paths.
Ok, so I see that this patch does a bit more than it may seem at first glance. I think it would be good to enumerate the list of scenarios that you're interested in here, because there are multiple quirks of different components that we are working with here, and each may require a fix elsewhere. The scenarios I know about (but there may be others) are:
1. breakpad truncating long build-ids
2. LLDB using a a crc checksum as a build-id, when the real build-id is missing
3. breakpad using some other arbitrary checksum when a real build-id is missing

Of these, 1. and 3. are a problem in the producer, and we cannot do anything about that except work around them. Doing that close to the source (i.e. ProcessMinidump) is reasonable. However, 2. is an lldb problem (I would say even a "bug"), and we certainly can do something to fix it instead of just covering it out. So, if there's any way to cut complexity here by fixing elf UUID computation, I think we should do that instead.

Now, cases 1. and 3. do result in UUIDs that are incompatible with how we use UUIDs elsewhere in lldb, so working around this by retrying the search without a UUID and then doing some auxiliary matching seems like a reasonable compromise. However, it's not clear to me why you need to strip the path too. Based on my reading of ModuleList::GetSharedModule <https://github.com/llvm-mirror/lldb/blob/master/source/Core/ModuleList.cpp#L856>, the directory component is ignored when searching "exec-search-paths", and so we shouldn't need to do anything special to make this case work. And if that's the case (if it isn't, then I'd like to understand why), then it seems reasonable to restrict the UUID-less retry to the cases where we know breakpad might have generated incompatible UUIDs. Restricting it to `ElfBuildId`-records seems like a good first-order approximation, but if there are other clues we could use for that, I'd say we should use them too.

So I'd propose to split this patch into two (or more?) independent changes:
1. See what it takes to make exec-search-paths lookup work in the "normal" (i.e., the file has a regular build-id, and everybody agrees on what it is) case. I did some simple experiments, and it seemed to work for me without any special modifications. So if there's a case that doesn't work it could be a bug elsewhere that needs fixing.
2. implement the retry logic to help the cases when breakpad produces "wrong" uuid.
3. (?) If any of this is hampered by lldb crc "uuid" (I don't think it should be because if you specify that you don't wan't to search by uuid, then it really doesn't matter what lldb thinks the uuid's module is), then let's see if we can change that logic.


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

https://reviews.llvm.org/D60001