[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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

So far just a bunch of random comments...



================
Comment at: lldb/include/lldb/Target/Platform.h:1055
 
+  /// Given an address of a binary, the platform may be able to
+  /// set the correct DynamicLoader plugin that should be used for
----------------
Are there any restrictions on when in the process lifecycle this should be called?  If so it would be good to call that out.


================
Comment at: lldb/source/Core/DynamicLoader.cpp:191
   ModuleSP module_sp;
   PlatformSP platform_sp = process->GetTarget().GetPlatform();
   Target &target = process->GetTarget();
----------------
This isn't your addition, but it seems dangerous to assume process will always be non-null here...


================
Comment at: lldb/source/Core/DynamicLoader.cpp:233
   if (module_sp.get()) {
+    // Ensure the Target has an architecture set in case
+    // we need it while processing this binary/eh_frame/debug info.
----------------
It seems weird that we would get all the way here, with a live process and everything, but not have a target architecture.  How does that happen?


================
Comment at: lldb/source/Plugins/DynamicLoader/Darwin-Kernel/DynamicLoaderDarwinKernel.cpp:724
+              ObjectFile::eStrataKernel) {
         if (m_uuid != exe_module_sp->GetUUID()) {
           // The user specified a kernel binary that has a different UUID than
----------------
This second test is redundant, isn't it?



================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp:747
+
+  // Give the generic methods, including possibly calling into DebugSymbols
+  // framework on macOS systems, a chance.
----------------
Is this comment accurate anymore?  You only get to this code if module_spec.GetUUID is NOT valid, in which case DebugSymbols isn't going to find anything.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp:928
+    return LLDB_INVALID_ADDRESS;
+  ModuleSP module_sp(new Module(ModuleSpec()));
+  ObjectContainerSP container_sp(
----------------
Do you really need to pass in a module_sp with a pretty much totally bogus module in it?  That seems weird to me.  The Module constructor does a bunch of work, all of which is going to fail with an empty ModuleSpec, so at some point CreateMemoryInstance is going to replace whatever is in this module_sp with something real.  So what's the point of passing it something it's going to throw away?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp:2214
+            x.consume_front("0x");
+            if (llvm::to_integer(x, vmaddr, 16))
+              m_binary_addresses.push_back(vmaddr);
----------------
llvm::to_integer(..., 16) doesn't handle strings that start with 0x?  That seems weak.


================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:597
           const bool notify = true;
+          if (GetTarget()
+                  .GetDebugger()
----------------
I'm a little surprised we don't have a platform at this point?

What happens if we had a Platform at this point, but it wasn't the one that knew how to load this binary?  That seems awkward.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133534/new/

https://reviews.llvm.org/D133534