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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4149
+    
+    lldbassert(matching_modules.GetSize() == 1);
+    ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));
----------------
labath wrote:
> This should be a regular assert according to <https://lldb.llvm.org/resources/contributing.html#error-handling-and-use-of-assertions-in-lldb>.
This assert will never trigger with the above code already checking for empty on line 4123 and then checking if size > 1 on line 4140. Remove it?


================
Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175
+    
+    if (object_file->GetFileSpec() != symbol_fspec) {
+      result.AppendWarning("there is a discrepancy between the module file "
----------------
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".


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

https://reviews.llvm.org/D73594