[Lldb-commits] [PATCH] D133534: Complete support of loading a darwin kernel over a live gdb-remote connection given the address of a mach-o fileset

[Jonas Devlieghere via Phabricator via lldb-commits]

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