amccarth marked 3 inline comments as done. amccarth added a comment. I backed out the TODO and the warn-and-continue behavior to expedite this patch which is just supposed to be a refactor for readability.
We can revisit the issue of debug symbol file names not matching the object file names in the future (if necessary). ================ Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4103 + // this point. + // TODO: Is this part worthwhile? `foo.exe` will never match `foo.pdb` + if (matching_modules.IsEmpty()) ---------------- labath wrote: > amccarth wrote: > > labath wrote: > > > This is not unreasonable in the non-pdb world. You can have a stripped > > > version of a file somewhere prepared for deployment to some device (or > > > downloaded from the device), and then you'll also have an unstripped > > > version of that file (with the same name) in some build folder. > > Sure, if you've got two files with the same name in two directories that's > > fine. > > > > But the loop tries peeling the extension off of the supplied file name and > > comparing it to the target's file name. I guess it's trying to match > > something like `foo.unstripped` to `foo`. Is that a typical thing? > Ah, sorry, I missed that part. > > Yes, this is also thing that's used sometimes (probably even more often than > the first thing). One of the conventions for automatically finding [[ > https://sourceware.org/gdb/onlinedocs/gdb/Separate-Debug-Files.html | > separate debug files ]] involves appending `.debug` to your file and then > placing it in a special folder. I guess this was added to enable one to do > "target symbols add /whereever/foo.debug" and have it match "foo". > > That said, stripping the extension in a loop does seem like overkill. OK, I'm removing the TODO in favor of a comment with the `foo` and `foo.debug` you gave. Whether it should be a loop or not can be addressed later. Whether we should do something that would also work for Windows (e.g., `foo.exe` matching `foo.pdb`) can also be considered later. This patch is just supposed to be a refactor for readability/debuggability. ================ Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175-4178 + if (object_file->GetFileSpec() != symbol_fspec) { + result.AppendWarning("there is a discrepancy between the module file " + "spec and the symbol file spec\n"); + } ---------------- labath wrote: > amccarth wrote: > > clayborg wrote: > > > amccarth wrote: > > > > amccarth wrote: > > > > > clayborg wrote: > > > > > > labath wrote: > > > > > > > This part is not good. Everywhere else except pdbs this means > > > > > > > that we were in fact unable to load the symbol file. What happens > > > > > > > is that if we cannot load the object file representing the symbol > > > > > > > file (at [[ > > > > > > > https://github.com/llvm/llvm-project/blob/master/lldb/source/Symbol/SymbolVendor.cpp#L48 > > > > > > > | SymbolVendor.cpp:48 ]]), we fall back to creating a SymbolFile > > > > > > > using the object file of the original module (line 52). > > > > > > > > > > > > > > The effect of that is that the symbol file creation will always > > > > > > > succeed, the previous checks are more-or-less useless, and the > > > > > > > only way to check if we are really using the symbols from the > > > > > > > file the user specified is to compare the file specs. > > > > > > > > > > > > > > Now, PDB symbol files are abusing this fallback, and reading the > > > > > > > symbols from the pdb files even though they were in fact asked to > > > > > > > read them from the executable file. This is why this may sound > > > > > > > like a "discrepancy" coming from the pdb world, but everywhere > > > > > > > else this just means that the symbol file was not actually loaded. > > > > > > This could also fail if the symbol file spec was a bundle which got > > > > > > resolved when the module was loaded. If the user specified > > > > > > "/path/to/foo.dSYM" and the underlying code was able to resolve the > > > > > > symbol file in the dSYM bundle to > > > > > > "/path/to/foo.dSYM/Contents/Resources/DWARF/foo". > > > > > Interesting. I did not expect that fallback behavior from the API. > > > > > > > > > > > PDB symbol files are abusing this fallback > > > > > > > > > > I'm not sure there's abuse going on here. I'm not understanding > > > > > something about how lldb models this situation. Consider the case > > > > > where the user explicitly asks lldb to load symbols from a PDB: > > > > > > > > > > (lldb) target add symbols D:\my_symbols\foo.pdb > > > > > > > > > > Then `symbol_file` is the PDB and `object_file` is the executable or > > > > > DLL. I would expect those file specs to match only in the case where > > > > > the symbols are in the binary (in other words, when symbol file and > > > > > object file are the same file). Even ignoring the PDB case, it seems > > > > > this wouldn't even work in the case you mentioned above where the > > > > > object file is stripped and the symbol file is an unstripped copy of > > > > > the object file (possibly with a different name). You can also get > > > > > here if the symbols were matched by UUID rather than file spec. Or > > > > > when the symbols were matched by the basename minus some extension. > > > > > > > > > > Given that dsyms work, I must be misunderstanding something here. > > > > > Can you help me understand? > > > > > > > > > > What's the right thing to do here? If I just undo this refactor, > > > > > then we're back to silent failures when the symbols exist in a > > > > > different file than the object. > > > > In your example, the file specs do not match. > > > > > > > > 1. The old code would therefore reject the symbol file and return an > > > > error. > > > > 2. The new code would therefore emit a warning but proceed with the > > > > selected symbol file and return success. > > > > > > > > Do you want either of these behaviors or something else? > > > all I know is I can and have typed: > > > ``` > > > (lldb) target symbols add /path/to/foo.dSYM > > > ``` > > > And it would successfully associate it with the binary, probably because > > > the code on 4071 would see a UUID and accept it before it gets to this > > > point. > > > > > > What is the point of the warning? When would this happen? > > There are examples in the thread with Pavel just below this thread. On > > Windows, your symbols may come from a file called `foo.pdb` for an > > executable called `foo.exe`. The old code would successfully load the PDB > > file, then decide it was no good because the file specs don't match. > > (Heck, at this point, we've just gone through a bunch of gyrations to allow > > matching of differing file specs.) So you'd get a mostly silent failure > > for code that actually worked. > > > > My intent was to simply warn about such cases without actually forcing a > > failure, and to do so with some explicit feedback so that the user > > understands. > > > > I'll wait to hear what Pavel says. If necessary, I'll just roll back this > > part of the change (so that this remains just a refactor), and I'll figure > > out a way around the problem in a later patch. > Greg's use case will work because the bundle path will be resolved to the > actual file (in PlatformDarwin::ResolveSymbolFile) before we get to this > place. > > Overall, I think changing this to a warning would be acceptable. Thinking > about this more, I am not sure if the scenario I described below could happen > (easily -- it might for some very weird elf files where we were able to > extract the uuid, but cannot create an actual ObjectFileELF instance). So > calling this a discrepancy would be fine. > > And I do think that the current pdb behavior is a big discrepancy, which > should be fixed. Either we should go the breakpad way, and introduce an > "ObjectFilePDB" so that the pdb symbol file plugin(s) can behave like the > dwarf code does, or we should change the SymbolFile class so that it does not > require an object file instance in order to read symbols from a separate file > (and then we can delete ObjectFileBreakpad). > > Having a more explicit method of checking whether the symbol file creation > was successful (instead of checking file specs) would not hurt either... Thanks for all the clarification. I am planning to introduce an ObjectFilePDB in a future patch to solve other problems. To keep this patch a behavior-preserving refactor, I'll back out the warning message. Once PDBs can be loaded reliably, I can circle back here to see if there's a better way to detect failure and/or to communicate matching problems to the user. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73594/new/ https://reviews.llvm.org/D73594 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
