[Lldb-commits] [PATCH] D150928: Two bug fixes for loading process save-core created core files, plus perf improvements

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Thanks for the review comments, really helpful.  I've also added a new block to `DynamicLoader::LoadBinaryWithUUIDAndAddress` to make the binary search system: (1) `Target::GetOrCreateModule` (lldb already has it), (2) `Symbols::LocateExecutableObjectFile` / `Symbols::LocateExecutableSymbolFile` (on a macOS system, this would be your Spotlight search), and then finally (3) `Symbols::DownloadObjectAndSymbolFile` with the `force_symbol_search` as passed in.  I need to retest this with these different use cases to make sure it's all working correctly still, but looking pretty good now.



================
Comment at: lldb/source/Core/DynamicLoader.cpp:216-218
+    if (!module_sp || !module_sp->GetSymbolFileFileSpec()) {
+      Symbols::DownloadObjectAndSymbolFile(module_spec, error,
+                                           force_symbol_search);
----------------
bulbazord wrote:
> I think this **technically** changes behavior and probably not in a desirable way. Specifically, we previously only called `DownloadObjectAndSymbolFile` if `force_symbol_search` was true. Now you're passing it through which makes sense looking at the interface of `DownloadObjectAndSymbolFile`. However, peeking at the beginning of that function we see this code block:
> ```
>   // When dbgshell_command is empty, the user has not enabled the use of an
>   // external program to find the symbols, don't run it for them.
>   if (!force_lookup && dbgshell_command.empty())
>     return false;
> ```
> This means that even if `force_lookup` is false (or `force_symbol_search` as it's written here), if `dbgshell_command` is non-empty we'll still perform the lookup (assuming all the other checks pass) which is probably not what we intended performance-wise. That condition in the block above should probably be something like `!force_lookup || dbgshell_command.empty()` instead of `&&`.
Good catch.  This is behavior I want, but it sure isn't real clear from the way this method is written.  I will add some comments to make it clear that we want to do this external-program-lookup if either `force_lookup` is set, or if the user set `dbgshell_command` manually in their com.apple.DebugSymbols preferences.


================
Comment at: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:7021
         }
-        const bool address_is_slide = true;
         bool changed = false;
+        module_sp->SetLoadAddress(process.GetTarget(), value, value_is_offset,
----------------
bulbazord wrote:
> nit: this can be const
it's not clearly written, this is actually an output parameter from Module::SetLoadAddress.  The obvious next question is - why is it initialized.  It should not be, I'll remove the initialization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150928