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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
