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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
