bulbazord added a comment.
Bug fixes look good to me, the perf and code simplifications are also great to
see. A few nits and I have one concern. Otherwise looks fine to me, I think.
================
Comment at: lldb/include/lldb/Target/DynamicLoader.h:270-273
+ LoadBinaryWithUUIDAndAddress(Process *process, llvm::StringRef name,
+ UUID uuid, lldb::addr_t value,
+ bool value_is_offset, bool force_symbol_search,
+ bool notify, bool set_address_in_target);
----------------
Just thinking aloud here, I wonder if we should construct some kind of
"Options" struct to contain all of these bools?
================
Comment at: lldb/source/Core/DynamicLoader.cpp:216-218
+ if (!module_sp || !module_sp->GetSymbolFileFileSpec()) {
+ Symbols::DownloadObjectAndSymbolFile(module_spec, error,
+ force_symbol_search);
----------------
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 `&&`.
================
Comment at: lldb/source/Core/DynamicLoader.cpp:246-248
+ LLDB_LOGF(log,
+ "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading "
+ "binary UUID %s at %s 0x%" PRIx64,
----------------
You could use `LLDB_LOG` instead of `LLDB_LOGF` here and you'll get the file +
function without explicitly writing it, but you'd have to change the formatting
a bit since it uses `Format` and not `Printf` under the hood. Just a suggestion
though.
================
Comment at: lldb/source/Core/DynamicLoader.cpp:255-257
+ LLDB_LOGF(log,
+ "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading "
+ "binary UUID %s at file address",
----------------
Same here
================
Comment at: lldb/source/Core/DynamicLoader.cpp:264-266
+ LLDB_LOGF(log,
+ "DynamicLoader::LoadBinaryWithUUIDAndAddress Loading binary "
+ "UUID %s from memory at address 0x%" PRIx64,
----------------
Also here
================
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,
----------------
nit: this can be const
================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1000
this, "", standalone_uuid, standalone_value,
- standalone_value_is_offset, force_symbol_search, notify);
+ standalone_value_is_offset, force_symbol_search, notify, true);
}
----------------
nit: `, /* set_address_in_target */ true);` (or something like this)
Or you can create a `const bool` like above with `force_symbol_search` and
`notify`.
================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1031
+ this, llvm::StringRef(), uuid, addr, value_is_slide,
+ force_symbol_search, notify, true);
}
----------------
Same here
================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:262-264
this, llvm::StringRef(), objfile_binary_uuid,
objfile_binary_value, objfile_binary_value_is_offset,
+ force_symbol_search, notify, true)) {
----------------
Same here
================
Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:318
this, llvm::StringRef(), ident_uuid, ident_binary_addr,
- value_is_offset, force_symbol_search, notify)) {
+ value_is_offset, force_symbol_search, notify, true)) {
found_main_binary_definitively = true;
----------------
Same here
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
