JDevlieghere added a comment. The current patch didn't have context so I just left a bunch of nits.
================ Comment at: lldb/include/lldb/Target/Platform.h:870-874 + virtual bool LoadSpecialBinaryAndSetDynamicLoader(Process *process, + lldb::addr_t addr, + bool notify) { + return false; + } ---------------- What makes the binary special? The comment above doesn't say. Can we give this a more descriptive name or omit the "Special" part? ================ Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:133 +static bool module_is_a_kernel(Module *module) { + if (!module) ---------------- This seems needless verbose: `is_kernel` conveys the same information. ================ Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:574-577 + if (module_is_a_kernel(module_sp.get())) + m_kernel_image = true; + else + m_kernel_image = false; ---------------- `m_kernel_image = is_kernel(module_sp.get())` ================ 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; ---------------- `is_kernel = is_kernel(memory_module_sp.get())` ================ 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); ---------------- The `ModuleList` has a `Modules()` function that returns a locking iterator. You can rewrite this as: ``` for(ModuleSP module : target.GetImages().Modules()) { ... } ``` ================ Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp:929 + return LLDB_INVALID_ADDRESS; + ModuleSP module_sp(new Module(ModuleSpec())); + ObjectContainerSP container_sp( ---------------- ```ModuleSP module_sp = std::make_shared<Module>()``` ================ Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp:970 + PlatformDarwinKernel::GetPluginNameStatic()); + if (platform_sp.get()) + process->GetTarget().SetPlatform(platform_sp); ---------------- ```if(platform_sp)``` ================ Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp:973-974 + + DynamicLoaderUP dyld_up( + new DynamicLoaderDarwinKernel(process, actual_address)); + if (!dyld_up) ---------------- std::make_shared 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