[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

[Jason Molenda via Phabricator via lldb-commits]

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