[Lldb-commits] [lldb] e67cee0 - [lldb] Avoid duplicate vdso modules when opening core files
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Tue Apr 5 02:23:16 PDT 2022
Author: Pavel Labath
Date: 2022-04-05T11:22:37+02:00
New Revision: e67cee09499cbf386334115a627cd4fbe19cb01c
URL: https://github.com/llvm/llvm-project/commit/e67cee09499cbf386334115a627cd4fbe19cb01c
DIFF: https://github.com/llvm/llvm-project/commit/e67cee09499cbf386334115a627cd4fbe19cb01c.diff
LOG: [lldb] Avoid duplicate vdso modules when opening core files
When opening core files (and also in some other situations) we could end
up with two vdso modules. This could happen because the vdso module is
very special, and over the years, we have accumulated various ways to
load it.
In D10800, we added one mechanism for loading it, which took the form of
a generic load-from-memory capability. Unfortunately loading an elf file
from memory is not possible (because the loader never loads the entire
file), and our attempts to do so were causing crashes. So, in D34352, we
partially reverted D10800 and implemented a custom mechanism specific to
the vdso.
Unfortunately, enough of D10800 remained such that, under the right
circumstances, it could end up loading a second (non-functional) copy of
the vdso module. This happened when the process plugin did not support
the extended MemoryRegionInfo query (added in D22219, to workaround a
different bug), which meant that the loader plugin was not able to
recognise that the linux-vdso.so.1 module (this is how the loader calls
it) is in fact the same as the [vdso] module (the name used in
/proc/$PID/maps) we loaded before. This typically happened in a core
file, as they don't store this kind of information.
This patch fixes the issue by completing the revert of D10800 -- the
memory loading code is removed completely. It also reduces the scope of
the hackaround introduced in D22219 -- it isn't completely sound and is
only relevant for fairly old (but still supported) versions of android.
I added the memory loading logic to the wasm dynamic loader, which has
since appeared and is relying on this feature (it even has a test). As
far as I can tell loading wasm modules from memory is possible and
reliable. MachO memory loading is not affected by this patch, as it uses
a completely different code path.
Since the scenarios/patches I described came without test cases, I have
created two new gdb-client tests cases for them. They're not
particularly readable, but right now, this is the best way we can
simulate the behavior (bugs) of a particular dynamic linker.
Differential Revision: https://reviews.llvm.org/D122660
Added:
lldb/test/API/functionalities/gdb_remote_client/TestGdbClientModuleLoad.py
lldb/test/API/functionalities/gdb_remote_client/module_load.yaml
Modified:
lldb/include/lldb/Target/DynamicLoader.h
lldb/packages/Python/lldbsuite/test/gdbclientutils.py
lldb/source/Core/DynamicLoader.cpp
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp
lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h
lldb/test/API/functionalities/gdb_remote_client/TestWasm.py
Removed:
################################################################################
diff --git a/lldb/include/lldb/Target/DynamicLoader.h b/lldb/include/lldb/Target/DynamicLoader.h
index 84ad0f11fabb5..4f07045f86457 100644
--- a/lldb/include/lldb/Target/DynamicLoader.h
+++ b/lldb/include/lldb/Target/DynamicLoader.h
@@ -263,6 +263,8 @@ class DynamicLoader : public PluginInterface {
protected:
// Utility methods for derived classes
+ lldb::ModuleSP FindModuleViaTarget(const FileSpec &file);
+
/// Checks to see if the target module has changed, updates the target
/// accordingly and returns the target executable module.
lldb::ModuleSP GetTargetExecutable();
diff --git a/lldb/packages/Python/lldbsuite/test/gdbclientutils.py b/lldb/packages/Python/lldbsuite/test/gdbclientutils.py
index 78854bb9dae87..68ac0e07b4a14 100644
--- a/lldb/packages/Python/lldbsuite/test/gdbclientutils.py
+++ b/lldb/packages/Python/lldbsuite/test/gdbclientutils.py
@@ -66,8 +66,9 @@ def hex_decode_bytes(hex_bytes):
"""
out = ""
hex_len = len(hex_bytes)
+ i = 0
while i < hex_len - 1:
- out += chr(int(hex_bytes[i:i + 2]), 16)
+ out += chr(int(hex_bytes[i:i + 2], 16))
i += 2
return out
@@ -178,6 +179,8 @@ def respond(self, packet):
return self.qGetWorkingDir()
if packet == "qOffsets":
return self.qOffsets();
+ if packet == "qProcessInfo":
+ return self.qProcessInfo()
if packet == "qsProcessInfo":
return self.qsProcessInfo()
if packet.startswith("qfProcessInfo"):
@@ -214,6 +217,9 @@ def qGetWorkingDir(self):
def qOffsets(self):
return ""
+ def qProcessInfo(self):
+ return ""
+
def qHostInfo(self):
return "ptrsize:8;endian:little;"
diff --git a/lldb/source/Core/DynamicLoader.cpp b/lldb/source/Core/DynamicLoader.cpp
index 1c7b9125e4d1b..36443228ecd1d 100644
--- a/lldb/source/Core/DynamicLoader.cpp
+++ b/lldb/source/Core/DynamicLoader.cpp
@@ -145,72 +145,31 @@ DynamicLoader::GetSectionListFromModule(const ModuleSP module) const {
return sections;
}
-ModuleSP DynamicLoader::LoadModuleAtAddress(const FileSpec &file,
- addr_t link_map_addr,
- addr_t base_addr,
- bool base_addr_is_offset) {
+ModuleSP DynamicLoader::FindModuleViaTarget(const FileSpec &file) {
Target &target = m_process->GetTarget();
- ModuleList &modules = target.GetImages();
ModuleSpec module_spec(file, target.GetArchitecture());
- ModuleSP module_sp;
- if ((module_sp = modules.FindFirstModule(module_spec))) {
- UpdateLoadedSections(module_sp, link_map_addr, base_addr,
- base_addr_is_offset);
+ if (ModuleSP module_sp = target.GetImages().FindFirstModule(module_spec))
return module_sp;
- }
- if ((module_sp = target.GetOrCreateModule(module_spec,
- true /* notify */))) {
- UpdateLoadedSections(module_sp, link_map_addr, base_addr,
- base_addr_is_offset);
+ if (ModuleSP module_sp = target.GetOrCreateModule(module_spec,
+ /*notify=*/true))
return module_sp;
- }
-
- bool check_alternative_file_name = true;
- if (base_addr_is_offset) {
- // Try to fetch the load address of the file from the process as we need
- // absolute load address to read the file out of the memory instead of a
- // load bias.
- bool is_loaded = false;
- lldb::addr_t load_addr;
- Status error = m_process->GetFileLoadAddress(file, is_loaded, load_addr);
- if (error.Success() && is_loaded) {
- check_alternative_file_name = false;
- base_addr = load_addr;
- }
- }
-
- // We failed to find the module based on its name. Lets try to check if we
- // can find a
diff erent name based on the memory region info.
- if (check_alternative_file_name) {
- MemoryRegionInfo memory_info;
- Status error = m_process->GetMemoryRegionInfo(base_addr, memory_info);
- if (error.Success() && memory_info.GetMapped() &&
- memory_info.GetRange().GetRangeBase() == base_addr &&
- !(memory_info.GetName().IsEmpty())) {
- ModuleSpec new_module_spec(FileSpec(memory_info.GetName().GetStringRef()),
- target.GetArchitecture());
-
- if ((module_sp = modules.FindFirstModule(new_module_spec))) {
- UpdateLoadedSections(module_sp, link_map_addr, base_addr, false);
- return module_sp;
- }
- if ((module_sp = target.GetOrCreateModule(new_module_spec,
- true /* notify */))) {
- UpdateLoadedSections(module_sp, link_map_addr, base_addr, false);
- return module_sp;
- }
- }
- }
+ return nullptr;
+}
- if ((module_sp = m_process->ReadModuleFromMemory(file, base_addr))) {
- UpdateLoadedSections(module_sp, link_map_addr, base_addr, false);
- target.GetImages().AppendIfNeeded(module_sp);
+ModuleSP DynamicLoader::LoadModuleAtAddress(const FileSpec &file,
+ addr_t link_map_addr,
+ addr_t base_addr,
+ bool base_addr_is_offset) {
+ if (ModuleSP module_sp = FindModuleViaTarget(file)) {
+ UpdateLoadedSections(module_sp, link_map_addr, base_addr,
+ base_addr_is_offset);
+ return module_sp;
}
- return module_sp;
+ return nullptr;
}
int64_t DynamicLoader::ReadUnsignedIntWithSizeInBytes(addr_t addr,
diff --git a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
index 12f438f79d043..7993c5906aa35 100644
--- a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -575,6 +575,38 @@ ModuleSP DynamicLoaderPOSIXDYLD::LoadInterpreterModule() {
return nullptr;
}
+ModuleSP DynamicLoaderPOSIXDYLD::LoadModuleAtAddress(const FileSpec &file,
+ addr_t link_map_addr,
+ addr_t base_addr,
+ bool base_addr_is_offset) {
+ if (ModuleSP module_sp = DynamicLoader::LoadModuleAtAddress(
+ file, link_map_addr, base_addr, base_addr_is_offset))
+ return module_sp;
+
+ // This works around an dynamic linker "bug" on android <= 23, where the
+ // dynamic linker would report the application name
+ // (e.g. com.example.myapplication) instead of the main process binary
+ // (/system/bin/app_process(32)). The logic is not sound in general (it
+ // assumes base_addr is the real address, even though it actually is a load
+ // bias), but it happens to work on adroid because app_process has a file
+ // address of zero.
+ // This should be removed after we drop support for android-23.
+ if (m_process->GetTarget().GetArchitecture().GetTriple().isAndroid()) {
+ MemoryRegionInfo memory_info;
+ Status error = m_process->GetMemoryRegionInfo(base_addr, memory_info);
+ if (error.Success() && memory_info.GetMapped() &&
+ memory_info.GetRange().GetRangeBase() == base_addr &&
+ !(memory_info.GetName().IsEmpty())) {
+ if (ModuleSP module_sp = DynamicLoader::LoadModuleAtAddress(
+ FileSpec(memory_info.GetName().GetStringRef()), link_map_addr,
+ base_addr, base_addr_is_offset))
+ return module_sp;
+ }
+ }
+
+ return nullptr;
+}
+
void DynamicLoaderPOSIXDYLD::LoadAllCurrentModules() {
DYLDRendezvous::iterator I;
DYLDRendezvous::iterator E;
diff --git a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
index 422856e7a6606..8d3e4cde54c72 100644
--- a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
+++ b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
@@ -55,6 +55,11 @@ class DynamicLoaderPOSIXDYLD : public lldb_private::DynamicLoader {
// PluginInterface protocol
llvm::StringRef GetPluginName() override { return GetPluginNameStatic(); }
+ lldb::ModuleSP LoadModuleAtAddress(const lldb_private::FileSpec &file,
+ lldb::addr_t link_map_addr,
+ lldb::addr_t base_addr,
+ bool base_addr_is_offset) override;
+
protected:
/// Runtime linker rendezvous structure.
DYLDRendezvous m_rendezvous;
diff --git a/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp b/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp
index 39d91c8abf09e..d019415cb67a6 100644
--- a/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp
+++ b/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.cpp
@@ -64,3 +64,19 @@ ThreadPlanSP DynamicLoaderWasmDYLD::GetStepThroughTrampolinePlan(Thread &thread,
bool stop) {
return ThreadPlanSP();
}
+
+lldb::ModuleSP DynamicLoaderWasmDYLD::LoadModuleAtAddress(
+ const lldb_private::FileSpec &file, lldb::addr_t link_map_addr,
+ lldb::addr_t base_addr, bool base_addr_is_offset) {
+ if (ModuleSP module_sp = DynamicLoader::LoadModuleAtAddress(
+ file, link_map_addr, base_addr, base_addr_is_offset))
+ return module_sp;
+
+ if (ModuleSP module_sp = m_process->ReadModuleFromMemory(file, base_addr)) {
+ UpdateLoadedSections(module_sp, link_map_addr, base_addr, false);
+ m_process->GetTarget().GetImages().AppendIfNeeded(module_sp);
+ return module_sp;
+ }
+
+ return nullptr;
+}
diff --git a/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h b/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h
index fe67e58859042..5ed855395cca7 100644
--- a/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h
+++ b/lldb/source/Plugins/DynamicLoader/wasm-DYLD/DynamicLoaderWasmDYLD.h
@@ -33,6 +33,11 @@ class DynamicLoaderWasmDYLD : public DynamicLoader {
Status CanLoadImage() override { return Status(); }
lldb::ThreadPlanSP GetStepThroughTrampolinePlan(Thread &thread,
bool stop) override;
+ lldb::ModuleSP LoadModuleAtAddress(const lldb_private::FileSpec &file,
+ lldb::addr_t link_map_addr,
+ lldb::addr_t base_addr,
+ bool base_addr_is_offset) override;
+
/// \}
/// PluginInterface protocol.
diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestGdbClientModuleLoad.py b/lldb/test/API/functionalities/gdb_remote_client/TestGdbClientModuleLoad.py
new file mode 100644
index 0000000000000..9a4c4cd059768
--- /dev/null
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestGdbClientModuleLoad.py
@@ -0,0 +1,133 @@
+import lldb
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+from lldbsuite.test.gdbclientutils import *
+from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase
+from lldbsuite.support import seven
+
+class MyResponder(MockGDBServerResponder):
+ """
+ A responder which simulates a process with a single shared library loaded.
+ Its parameters allow configuration of various properties of the library.
+ """
+
+ def __init__(self, testcase, triple, library_name, auxv_entry, region_info):
+ MockGDBServerResponder.__init__(self)
+ self.testcase = testcase
+ self._triple = triple
+ self._library_name = library_name
+ self._auxv_entry = auxv_entry
+ self._region_info = region_info
+
+ def qSupported(self, client_supported):
+ return (super().qSupported(client_supported) +
+ ";qXfer:auxv:read+;qXfer:libraries-svr4:read+")
+
+ def qXferRead(self, obj, annex, offset, length):
+ if obj == "features" and annex == "target.xml":
+ return """<?xml version="1.0"?>
+ <target version="1.0">
+ <architecture>i386:x86-64</architecture>
+ <feature name="org.gnu.gdb.i386.core">
+ <reg name="rip" bitsize="64" regnum="0" type="code_ptr" group="general"/>
+ </feature>
+ </target>""", False
+ elif obj == "auxv":
+ # 0x09 = AT_ENTRY, which lldb uses to compute the load bias of the
+ # main binary.
+ return hex_decode_bytes(self._auxv_entry +
+ "09000000000000000000ee000000000000000000000000000000000000000000"), False
+ elif obj == "libraries-svr4":
+ return """<?xml version="1.0"?>
+ <library-list-svr4 version="1.0">
+ <library name="%s" lm="0xdeadbeef" l_addr="0xef0000" l_ld="0xdeadbeef"/>
+ </library-list-svr4>""" % self._library_name, False
+ else:
+ return None, False
+
+ def qfThreadInfo(self):
+ return "m47"
+
+ def qsThreadInfo(self):
+ return "l"
+
+ def qProcessInfo(self):
+ return "pid:47;ptrsize:8;endian:little;triple:%s;" % hex_encode_bytes(self._triple)
+
+ def setBreakpoint(self, packet):
+ return "OK"
+
+ def readMemory(self, addr, length):
+ if addr == 0xee1000:
+ return "00"*0x30 + "0020ee0000000000"
+ elif addr == 0xee2000:
+ return "01000000000000000030ee0000000000dead00000000000000000000000000000000000000000000"
+ elif addr == 0xef0000:
+ with open(self.testcase.getBuildArtifact("libmodule_load.so"), "rb") as f:
+ contents = f.read(-1)
+ return hex_encode_bytes(seven.bitcast_to_string(contents))
+ return ("baadf00d00"*1000)[0:length*2]
+
+ def qMemoryRegionInfo(self, addr):
+ if addr < 0xee0000:
+ return "start:0;size:ee0000;"
+ elif addr < 0xef0000:
+ return "start:ee0000;size:10000;"
+ elif addr < 0xf00000:
+ return "start:ef0000;size:1000;permissions:rx;" + self._region_info
+ else:
+ return "start:ef1000;size:ffffffffff10f000"
+
+class TestGdbClientModuleLoad(GDBRemoteTestBase):
+
+ mydir = TestBase.compute_mydir(__file__)
+
+ @skipIfXmlSupportMissing
+ def test_android_app_process(self):
+ """
+ This test simulates the scenario where the (android) dynamic linker
+ reports incorrect file name of the main executable. Lldb uses
+ qMemoryRegionInfo to get the correct value.
+ """
+ region_info = "name:%s;" % (
+ hex_encode_bytes(self.getBuildArtifact("libmodule_load.so")))
+ self.server.responder = MyResponder(self, "x86_64-pc-linux-android",
+ "bogus-name", "", region_info)
+ self.yaml2obj("module_load.yaml", self.getBuildArtifact("libmodule_load.so"))
+ target = self.createTarget("module_load.yaml")
+
+ process = self.connect(target)
+ self.assertTrue(process.IsValid(), "Process is valid")
+
+ lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+ [lldb.eStateStopped])
+
+ self.filecheck("image list", __file__, "-check-prefix=ANDROID")
+# ANDROID: [ 0] {{.*}} 0x0000000000ee0000 {{.*}}module_load
+# ANDROID: [ 1] {{.*}} 0x0000000000ef0000 {{.*}}libmodule_load.so
+
+ @skipIfXmlSupportMissing
+ def test_vdso(self):
+ """
+ This test checks vdso loading in the situation where the process does
+ not have memory region information about the vdso address. This can
+ happen in core files, as they don't store this data.
+ We want to check that the vdso is loaded exactly once.
+ """
+ # vdso address
+ AT_SYSINFO_EHDR = "21000000000000000000ef0000000000"
+ self.server.responder = MyResponder(self, "x86_64-pc-linux",
+ "linux-vdso.so.1", AT_SYSINFO_EHDR, "")
+ self.yaml2obj("module_load.yaml", self.getBuildArtifact("libmodule_load.so"))
+ target = self.createTarget("module_load.yaml")
+
+ process = self.connect(target)
+ self.assertTrue(process.IsValid(), "Process is valid")
+
+ lldbutil.expect_state_changes(self, self.dbg.GetListener(), process,
+ [lldb.eStateStopped])
+
+ self.filecheck("image list", __file__, "-check-prefix=VDSO")
+# VDSO: [ 0] {{.*}} 0x0000000000ee0000 {{.*}}module_load
+# VDSO: [ 1] {{.*}} 0x0000000000ef0000 {{.*}}[vdso]
+ self.assertEquals(self.target().GetNumModules(), 2)
diff --git a/lldb/test/API/functionalities/gdb_remote_client/TestWasm.py b/lldb/test/API/functionalities/gdb_remote_client/TestWasm.py
index 14e71642a4ac1..0d94e78883587 100644
--- a/lldb/test/API/functionalities/gdb_remote_client/TestWasm.py
+++ b/lldb/test/API/functionalities/gdb_remote_client/TestWasm.py
@@ -32,8 +32,6 @@ def __init__(self, obj_path, module_name = ""):
MockGDBServerResponder.__init__(self)
def respond(self, packet):
- if packet == "qProcessInfo":
- return self.qProcessInfo()
if packet[0:13] == "qRegisterInfo":
return self.qRegisterInfo(packet[13:])
return MockGDBServerResponder.respond(self, packet)
diff --git a/lldb/test/API/functionalities/gdb_remote_client/module_load.yaml b/lldb/test/API/functionalities/gdb_remote_client/module_load.yaml
new file mode 100644
index 0000000000000..5410edc0458c6
--- /dev/null
+++ b/lldb/test/API/functionalities/gdb_remote_client/module_load.yaml
@@ -0,0 +1,53 @@
+--- !ELF
+FileHeader:
+ Class: ELFCLASS64
+ Data: ELFDATA2LSB
+ Type: ET_EXEC
+ Machine: EM_X86_64
+Sections:
+ - Name: .text
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC, SHF_EXECINSTR ]
+ Address: 0x0000
+ AddressAlign: 0x4
+ Content: "c3c3c3c3"
+ - Name: .data
+ Type: SHT_PROGBITS
+ Flags: [ SHF_ALLOC ]
+ Address: 0x1000
+ AddressAlign: 0x4
+ Size: 0x28
+ - Name: .dynamic
+ Type: SHT_DYNAMIC
+ Flags: [ SHF_WRITE, SHF_ALLOC ]
+ AddressAlign: 0x8
+ Entries:
+ - Tag: DT_DEBUG
+ Value: 0xdead0000
+
+ProgramHeaders:
+ - Type: PT_LOAD
+ Flags: [ PF_X, PF_R ]
+ VAddr: 0x0000
+ Align: 0x4
+ FirstSec: .text
+ LastSec: .text
+ - Type: PT_LOAD
+ Flags: [ PF_R, PF_W ]
+ VAddr: 0x1000
+ Align: 0x4
+ FirstSec: .data
+ LastSec: .dynamic
+ - Type: PT_DYNAMIC
+ Flags: [ PF_W, PF_R ]
+ VAddr: 0x1028
+ FirstSec: .dynamic
+ LastSec: .dynamic
+DynamicSymbols:
+ - Name: _r_debug
+ Type: STT_OBJECT
+ Section: .data
+ Binding: STB_GLOBAL
+ Value: 0x0
+ Size: 0x28
+
More information about the lldb-commits
mailing list