jasonmolenda marked 8 inline comments as done.
jasonmolenda added inline comments.
================
Comment at: lldb/include/lldb/Target/Platform.h:870-874
+ virtual bool LoadSpecialBinaryAndSetDynamicLoader(Process *process,
+ lldb::addr_t addr,
+ bool notify) {
+ return false;
+ }
----------------
JDevlieghere wrote:
> What makes the binary special? The comment above doesn't say. Can we give
> this a more descriptive name or omit the "Special" part?
I'm still not happy with this method name, I agree this name isn't clear. I'll
try to think of something better.
================
Comment at:
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:133
+static bool module_is_a_kernel(Module *module) {
+ if (!module)
----------------
JDevlieghere wrote:
> This seems needless verbose: `is_kernel` conveys the same information.
Good point, will do.
================
Comment at:
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:668-669
bool is_kernel = false;
- if (memory_module_sp->GetObjectFile()) {
- if (memory_module_sp->GetObjectFile()->GetType() ==
- ObjectFile::eTypeExecutable &&
- memory_module_sp->GetObjectFile()->GetStrata() ==
- ObjectFile::eStrataKernel) {
- is_kernel = true;
- } else if (memory_module_sp->GetObjectFile()->GetType() ==
- ObjectFile::eTypeSharedLibrary) {
- is_kernel = false;
- }
- }
+ if (module_is_a_kernel(memory_module_sp.get()))
+ is_kernel = true;
----------------
JDevlieghere wrote:
> `is_kernel = is_kernel(memory_module_sp.get())`
ah yes, it made more sense previously when the conditional expression was
larger.
================
Comment at:
lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:747-752
+ ModuleList module_list = target.GetImages();
+ std::lock_guard<std::recursive_mutex> guard(module_list.GetMutex());
+ const size_t num_modules = module_list.GetSize();
+ ModuleList incorrect_kernels;
+ for (size_t i = 0; i < num_modules; i++) {
+ ModuleSP module_sp = module_list.GetModuleAtIndex(i);
----------------
JDevlieghere wrote:
> The `ModuleList` has a `Modules()` function that returns a locking iterator.
> You can rewrite this as:
>
> ```
> for(ModuleSP module : target.GetImages().Modules()) {
> ...
> }
> ```
Ach, thanks, missed that.
================
Comment at: lldb/unittests/Interpreter/CMakeLists.txt:17-18
lldbInterpreter
+ lldbPluginDynamicLoaderDarwinKernel
+ lldbPluginObjectContainerMachOFileset
lldbPluginPlatformMacOSX
----------------
labath wrote:
> These dependencies should be added to the PlatformDarwinKernel
> CMakeLists.txt, not to the every file that depends on it.
Thanks Pavel, I should have thought of that.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D133534/new/
https://reviews.llvm.org/D133534
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
