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