[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

[Adrian McCarthy via Phabricator via lldb-commits]

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