LLDB Debuginfod tests and a fix or two #90622
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 7 commits
April 30, 2024 07:43
This reverts commit 7fc2fbb.
…Arch64/Arm buildbots
|
✅ With the latest revision this PR passed the Python code formatter. |
kevinfrei
changed the title
|
@llvm/pr-subscribers-lldb Author: Kevin Frei (kevinfrei) ChangesI'm taking yet another swing at getting these tests going, on the hypothesis that the problems with buildbots & whatnot are because they're not configured with CURL support, which I've confirmed would cause the previous tests to fail. (I have no access to an ARM64 linux system, but I did repro the failure on MacOS configured without CURL support) So, the only difference between this diff and previous diffs that have already been approved is that I've added a condition to the tests to only run if Debuginfod capabilities should be built into the binary. I had done this for these tests when they were Shell tests and not API tests, but I couldn't find a direct analog in any API test, so I used the "plugins" model used by the intel-pt tests as well. Patch is 26.05 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/90622.diff 12 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/make/Makefile.rules b/lldb/packages/Python/lldbsuite/test/make/Makefile.rules
index bfd249ccd43f2e..ee8793fa1b3029 100644
--- a/lldb/packages/Python/lldbsuite/test/make/Makefile.rules
+++ b/lldb/packages/Python/lldbsuite/test/make/Makefile.rules
@@ -51,7 +51,7 @@ LLDB_BASE_DIR := $(THIS_FILE_DIR)/../../../../../
#
# GNUWin32 uname gives "windows32" or "server version windows32" while
# some versions of MSYS uname return "MSYS_NT*", but most environments
-# standardize on "Windows_NT", so we'll make it consistent here.
+# standardize on "Windows_NT", so we'll make it consistent here.
# When running tests from Visual Studio, the environment variable isn't
# inherited all the way down to the process spawned for make.
#----------------------------------------------------------------------
@@ -210,6 +210,12 @@ else
ifeq "$(SPLIT_DEBUG_SYMBOLS)" "YES"
DSYM = $(EXE).debug
endif
+
+ ifeq "$(MAKE_DWP)" "YES"
+ MAKE_DWO := YES
+ DWP_NAME = $(EXE).dwp
+ DYLIB_DWP_NAME = $(DYLIB_NAME).dwp
+ endif
endif
LIMIT_DEBUG_INFO_FLAGS =
@@ -357,6 +363,7 @@ ifneq "$(OS)" "Darwin"
OBJCOPY ?= $(call replace_cc_with,objcopy)
ARCHIVER ?= $(call replace_cc_with,ar)
+ DWP ?= $(call replace_cc_with,dwp)
override AR = $(ARCHIVER)
endif
@@ -527,6 +534,10 @@ ifneq "$(CXX)" ""
endif
endif
+ifeq "$(GEN_GNU_BUILD_ID)" "YES"
+ LDFLAGS += -Wl,--build-id
+endif
+
#----------------------------------------------------------------------
# DYLIB_ONLY variable can be used to skip the building of a.out.
# See the sections below regarding dSYM file as well as the building of
@@ -565,10 +576,17 @@ else
endif
else
ifeq "$(SPLIT_DEBUG_SYMBOLS)" "YES"
+ifeq "$(SAVE_FULL_DEBUG_BINARY)" "YES"
+ cp "$(EXE)" "$(EXE).unstripped"
+endif
$(OBJCOPY) --only-keep-debug "$(EXE)" "$(DSYM)"
$(OBJCOPY) --strip-debug --add-gnu-debuglink="$(DSYM)" "$(EXE)" "$(EXE)"
endif
+ifeq "$(MAKE_DWP)" "YES"
+ $(DWP) -o "$(DWP_NAME)" $(DWOS)
endif
+endif
+
#----------------------------------------------------------------------
# Make the dylib
@@ -610,9 +628,15 @@ endif
else
$(LD) $(DYLIB_OBJECTS) $(LDFLAGS) -shared -o "$(DYLIB_FILENAME)"
ifeq "$(SPLIT_DEBUG_SYMBOLS)" "YES"
+ ifeq "$(SAVE_FULL_DEBUG_BINARY)" "YES"
+ cp "$(DYLIB_FILENAME)" "$(DYLIB_FILENAME).unstripped"
+ endif
$(OBJCOPY) --only-keep-debug "$(DYLIB_FILENAME)" "$(DYLIB_FILENAME).debug"
$(OBJCOPY) --strip-debug --add-gnu-debuglink="$(DYLIB_FILENAME).debug" "$(DYLIB_FILENAME)" "$(DYLIB_FILENAME)"
endif
+ifeq "$(MAKE_DWP)" "YES"
+ $(DWP) -o $(DYLIB_DWP_FILE) $(DYLIB_DWOS)
+endif
endif
#----------------------------------------------------------------------
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index 49f13d2c89e380..dafdf241f9db0c 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -4378,26 +4378,38 @@ const std::shared_ptr<SymbolFileDWARFDwo> &SymbolFileDWARF::GetDwpSymbolFile() {
FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
ModuleSpec module_spec;
module_spec.GetFileSpec() = m_objfile_sp->GetFileSpec();
+ FileSpec dwp_filespec;
for (const auto &symfile : symfiles.files()) {
module_spec.GetSymbolFileSpec() =
FileSpec(symfile.GetPath() + ".dwp", symfile.GetPathStyle());
LLDB_LOG(log, "Searching for DWP using: \"{0}\"",
module_spec.GetSymbolFileSpec());
- FileSpec dwp_filespec =
+ dwp_filespec =
PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
if (FileSystem::Instance().Exists(dwp_filespec)) {
- LLDB_LOG(log, "Found DWP file: \"{0}\"", dwp_filespec);
- DataBufferSP dwp_file_data_sp;
- lldb::offset_t dwp_file_data_offset = 0;
- ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
- GetObjectFile()->GetModule(), &dwp_filespec, 0,
- FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
- dwp_file_data_offset);
- if (dwp_obj_file) {
- m_dwp_symfile = std::make_shared<SymbolFileDWARFDwo>(
- *this, dwp_obj_file, DIERef::k_file_index_mask);
- break;
- }
+ break;
+ }
+ }
+ if (!FileSystem::Instance().Exists(dwp_filespec)) {
+ LLDB_LOG(log, "No DWP file found locally");
+ // Fill in the UUID for the module we're trying to match for, so we can
+ // find the correct DWP file, as the Debuginfod plugin uses *only* this
+ // data to correctly match the DWP file with the binary.
+ module_spec.GetUUID() = m_objfile_sp->GetUUID();
+ dwp_filespec =
+ PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
+ }
+ if (FileSystem::Instance().Exists(dwp_filespec)) {
+ LLDB_LOG(log, "Found DWP file: \"{0}\"", dwp_filespec);
+ DataBufferSP dwp_file_data_sp;
+ lldb::offset_t dwp_file_data_offset = 0;
+ ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
+ GetObjectFile()->GetModule(), &dwp_filespec, 0,
+ FileSystem::Instance().GetByteSize(dwp_filespec), dwp_file_data_sp,
+ dwp_file_data_offset);
+ if (dwp_obj_file) {
+ m_dwp_symfile = std::make_shared<SymbolFileDWARFDwo>(
+ *this, dwp_obj_file, DIERef::k_file_index_mask);
}
}
if (!m_dwp_symfile) {
diff --git a/lldb/source/Plugins/SymbolLocator/CMakeLists.txt b/lldb/source/Plugins/SymbolLocator/CMakeLists.txt
index ca969626f4ffc4..3367022639ab85 100644
--- a/lldb/source/Plugins/SymbolLocator/CMakeLists.txt
+++ b/lldb/source/Plugins/SymbolLocator/CMakeLists.txt
@@ -1,5 +1,10 @@
+# Order matters here: the first symbol locator prevents further searching.
+# For DWARF binaries that are both stripped and split, the Default plugin
+# will return the stripped binary when asked for the ObjectFile, which then
+# prevents an unstripped binary from being requested from the Debuginfod
+# provider.
+add_subdirectory(Debuginfod)
add_subdirectory(Default)
if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
add_subdirectory(DebugSymbols)
endif()
-add_subdirectory(Debuginfod)
diff --git a/lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp b/lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
index b5fe35d71032a8..f296e655cc466d 100644
--- a/lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
+++ b/lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
@@ -44,6 +44,24 @@ llvm::StringRef SymbolVendorELF::GetPluginDescriptionStatic() {
"executables.";
}
+// If this is needed elsewhere, it can be exported/moved.
+static bool IsDwpSymbolFile(const lldb::ModuleSP &module_sp,
+ const FileSpec &file_spec) {
+ DataBufferSP dwp_file_data_sp;
+ lldb::offset_t dwp_file_data_offset = 0;
+ // Try to create an ObjectFile from the file_spec.
+ ObjectFileSP dwp_obj_file = ObjectFile::FindPlugin(
+ module_sp, &file_spec, 0, FileSystem::Instance().GetByteSize(file_spec),
+ dwp_file_data_sp, dwp_file_data_offset);
+ // The presence of a debug_cu_index section is the key identifying feature of
+ // a DWP file. Make sure we don't fill in the section list on dwp_obj_file
+ // (by calling GetSectionList(false)) as this function could be called before
+ // we may have all the symbol files collected and available.
+ return dwp_obj_file && ObjectFileELF::classof(dwp_obj_file.get()) &&
+ dwp_obj_file->GetSectionList(false)->FindSectionByType(
+ eSectionTypeDWARFDebugCuIndex, false);
+}
+
// CreateInstance
//
// Platforms can register a callback to use when creating symbol vendors to
@@ -87,8 +105,15 @@ SymbolVendorELF::CreateInstance(const lldb::ModuleSP &module_sp,
FileSpecList search_paths = Target::GetDefaultDebugFileSearchPaths();
FileSpec dsym_fspec =
PluginManager::LocateExecutableSymbolFile(module_spec, search_paths);
- if (!dsym_fspec)
- return nullptr;
+ if (!dsym_fspec || IsDwpSymbolFile(module_sp, dsym_fspec)) {
+ // If we have a stripped binary or if we got a DWP file, we should prefer
+ // symbols in the executable acquired through a plugin.
+ ModuleSpec unstripped_spec =
+ PluginManager::LocateExecutableObjectFile(module_spec);
+ if (!unstripped_spec)
+ return nullptr;
+ dsym_fspec = unstripped_spec.GetFileSpec();
+ }
DataBufferSP dsym_file_data_sp;
lldb::offset_t dsym_file_data_offset = 0;
diff --git a/lldb/test/API/debuginfod/Normal/Makefile b/lldb/test/API/debuginfod/Normal/Makefile
new file mode 100644
index 00000000000000..54bd7adae241f5
--- /dev/null
+++ b/lldb/test/API/debuginfod/Normal/Makefile
@@ -0,0 +1,19 @@
+C_SOURCES := main.c
+
+# For normal (non DWP) Debuginfod tests, we need:
+
+# * The full binary: a.out.unstripped
+# Produced by Makefile.rules with SAVE_FULL_DEBUG_BINARY set to YES and
+# SPLIT_DEBUG_SYMBOLS set to YES
+
+# * The stripped binary (a.out)
+# Produced by Makefile.rules with SPLIT_DEBUG_SYMBOLS set to YES
+
+# * The 'only-keep-debug' binary (a.out.debug)
+# Produced below
+
+SPLIT_DEBUG_SYMBOLS := YES
+SAVE_FULL_DEBUG_BINARY := YES
+GEN_GNU_BUILD_ID := YES
+
+include Makefile.rules
diff --git a/lldb/test/API/debuginfod/Normal/TestDebuginfod.py b/lldb/test/API/debuginfod/Normal/TestDebuginfod.py
new file mode 100644
index 00000000000000..72c76be3972407
--- /dev/null
+++ b/lldb/test/API/debuginfod/Normal/TestDebuginfod.py
@@ -0,0 +1,183 @@
+import os
+import shutil
+import tempfile
+
+import lldb
+from lldbsuite.test.decorators import *
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+"""
+Test support for the DebugInfoD network symbol acquisition protocol.
+This one is for simple / no split-dwarf scenarios.
+
+For no-split-dwarf scenarios, there are 2 variations:
+1 - A stripped binary with it's corresponding unstripped binary:
+2 - A stripped binary with a corresponding --only-keep-debug symbols file
+"""
+
+
+class DebugInfodTests(TestBase):
+ # No need to try every flavor of debug inf.
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def setUp(self):
+ TestBase.setUp(self)
+ # Don't run these tests if we don't have Debuginfod support
+ if "Debuginfod" not in configuration.enabled_plugins:
+ self.skipTest("The Debuginfod SymbolLocator plugin is not enabled")
+
+ def test_normal_no_symbols(self):
+ """
+ Validate behavior with no symbols or symbol locator.
+ ('baseline negative' behavior)
+ """
+ test_root = self.config_test(["a.out"])
+ self.try_breakpoint(False)
+
+ def test_normal_default(self):
+ """
+ Validate behavior with symbols, but no symbol locator.
+ ('baseline positive' behavior)
+ """
+ test_root = self.config_test(["a.out", "a.out.debug"])
+ self.try_breakpoint(True)
+
+ def test_debuginfod_symbols(self):
+ """
+ Test behavior with the full binary available from Debuginfod as
+ 'debuginfo' from the plug-in.
+ """
+ test_root = self.config_test(["a.out"], "a.out.unstripped")
+ self.try_breakpoint(True)
+
+ def test_debuginfod_executable(self):
+ """
+ Test behavior with the full binary available from Debuginfod as
+ 'executable' from the plug-in.
+ """
+ test_root = self.config_test(["a.out"], None, "a.out.unstripped")
+ self.try_breakpoint(True)
+
+ def test_debuginfod_okd_symbols(self):
+ """
+ Test behavior with the 'only-keep-debug' symbols available from Debuginfod.
+ """
+ test_root = self.config_test(["a.out"], "a.out.debug")
+ self.try_breakpoint(True)
+
+ def try_breakpoint(self, should_have_loc):
+ """
+ This function creates a target from self.aout, sets a function-name
+ breakpoint, and checks to see if we have a file/line location,
+ as a way to validate that the symbols have been loaded.
+ should_have_loc specifies if we're testing that symbols have or
+ haven't been loaded.
+ """
+ target = self.dbg.CreateTarget(self.aout)
+ self.assertTrue(target and target.IsValid(), "Target is valid")
+
+ bp = target.BreakpointCreateByName("func")
+ self.assertTrue(bp and bp.IsValid(), "Breakpoint is valid")
+ self.assertEqual(bp.GetNumLocations(), 1)
+
+ loc = bp.GetLocationAtIndex(0)
+ self.assertTrue(loc and loc.IsValid(), "Location is valid")
+ addr = loc.GetAddress()
+ self.assertTrue(addr and addr.IsValid(), "Loc address is valid")
+ line_entry = addr.GetLineEntry()
+ self.assertEqual(
+ should_have_loc,
+ line_entry != None and line_entry.IsValid(),
+ "Loc line entry is valid",
+ )
+ if should_have_loc:
+ self.assertEqual(line_entry.GetLine(), 4)
+ self.assertEqual(
+ line_entry.GetFileSpec().GetFilename(),
+ self.main_source_file.GetFilename(),
+ )
+ self.dbg.DeleteTarget(target)
+ shutil.rmtree(self.tmp_dir)
+
+ def config_test(self, local_files, debuginfo=None, executable=None):
+ """
+ Set up a test with local_files[] copied to a different location
+ so that we control which files are, or are not, found in the file system.
+ Also, create a stand-alone file-system 'hosted' debuginfod server with the
+ provided debuginfo and executable files (if they exist)
+
+ Make the filesystem look like:
+
+ /tmp/<tmpdir>/test/[local_files]
+
+ /tmp/<tmpdir>/cache (for lldb to use as a temp cache)
+
+ /tmp/<tmpdir>/buildid/<uuid>/executable -> <executable>
+ /tmp/<tmpdir>/buildid/<uuid>/debuginfo -> <debuginfo>
+ Returns the /tmp/<tmpdir> path
+ """
+
+ self.build()
+
+ uuid = self.getUUID("a.out")
+ if not uuid:
+ self.fail("Could not get UUID for a.out")
+ return
+ self.main_source_file = lldb.SBFileSpec("main.c")
+ self.tmp_dir = tempfile.mkdtemp()
+ test_dir = os.path.join(self.tmp_dir, "test")
+ os.makedirs(test_dir)
+
+ self.aout = ""
+ # Copy the files used by the test:
+ for f in local_files:
+ shutil.copy(self.getBuildArtifact(f), test_dir)
+ # The first item is the binary to be used for the test
+ if self.aout == "":
+ self.aout = os.path.join(test_dir, f)
+
+ use_debuginfod = debuginfo != None or executable != None
+
+ # Populated the 'file://... mocked' Debuginfod server:
+ if use_debuginfod:
+ os.makedirs(os.path.join(self.tmp_dir, "cache"))
+ uuid_dir = os.path.join(self.tmp_dir, "buildid", uuid)
+ os.makedirs(uuid_dir)
+ if debuginfo:
+ shutil.copy(
+ self.getBuildArtifact(debuginfo),
+ os.path.join(uuid_dir, "debuginfo"),
+ )
+ if executable:
+ shutil.copy(
+ self.getBuildArtifact(executable),
+ os.path.join(uuid_dir, "executable"),
+ )
+
+ # Configure LLDB for the test:
+ self.runCmd(
+ "settings set symbols.enable-external-lookup %s"
+ % str(use_debuginfod).lower()
+ )
+ self.runCmd("settings clear plugin.symbol-locator.debuginfod.server-urls")
+ if use_debuginfod:
+ self.runCmd(
+ "settings set plugin.symbol-locator.debuginfod.cache-path %s/cache"
+ % self.tmp_dir
+ )
+ self.runCmd(
+ "settings insert-before plugin.symbol-locator.debuginfod.server-urls 0 file://%s"
+ % self.tmp_dir
+ )
+
+ def getUUID(self, filename):
+ try:
+ target = self.dbg.CreateTarget(self.getBuildArtifact(filename))
+ module = target.GetModuleAtIndex(0)
+ uuid = module.GetUUIDString().replace("-", "").lower()
+ self.dbg.DeleteTarget(target)
+ return uuid if len(uuid) == 40 else None
+ except:
+ return None
diff --git a/lldb/test/API/debuginfod/Normal/main.c b/lldb/test/API/debuginfod/Normal/main.c
new file mode 100644
index 00000000000000..4c7184609b4536
--- /dev/null
+++ b/lldb/test/API/debuginfod/Normal/main.c
@@ -0,0 +1,7 @@
+// This is a dump little pair of test files
+
+int func(int argc, const char *argv[]) {
+ return (argc + 1) * (argv[argc][0] + 2);
+}
+
+int main(int argc, const char *argv[]) { return func(0, argv); }
diff --git a/lldb/test/API/debuginfod/SplitDWARF/Makefile b/lldb/test/API/debuginfod/SplitDWARF/Makefile
new file mode 100644
index 00000000000000..3ab9a969e5a447
--- /dev/null
+++ b/lldb/test/API/debuginfod/SplitDWARF/Makefile
@@ -0,0 +1,23 @@
+C_SOURCES := main.c
+
+# For split-dwarf Debuginfod tests, we need:
+
+# * A .DWP file (a.out.dwp)
+# Produced by Makefile.rules with MAKE_DWP set to YES
+
+# * The "full" binary (missing things that live in .dwo's) (a.out.unstripped)
+# Produced by Makefile.rules with SAVE_FULL_DEBUG_BINARY set to YES and
+# SPLIT_DEBUG_SYMBOLS set to YES
+
+# * The stripped binary (a.out)
+# Produced by Makefile.rules
+
+# * The 'only-keep-debug' binary (a.out.debug)
+# Produced below
+
+MAKE_DWP := YES
+SPLIT_DEBUG_SYMBOLS := YES
+SAVE_FULL_DEBUG_BINARY := YES
+GEN_GNU_BUILD_ID := YES
+
+include Makefile.rules
diff --git a/lldb/test/API/debuginfod/SplitDWARF/TestDebuginfodDWP.py b/lldb/test/API/debuginfod/SplitDWARF/TestDebuginfodDWP.py
new file mode 100644
index 00000000000000..b9ab481ad159f4
--- /dev/null
+++ b/lldb/test/API/debuginfod/SplitDWARF/TestDebuginfodDWP.py
@@ -0,0 +1,192 @@
+"""
+Test support for the DebugInfoD network symbol acquisition protocol.
+"""
+import os
+import shutil
+import tempfile
+
+import lldb
+from lldbsuite.test.decorators import *
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+"""
+Test support for the DebugInfoD network symbol acquisition protocol.
+This file is for split-dwarf (dwp) scenarios.
+
+1 - A split binary target with it's corresponding DWP file
+2 - A stripped, split binary target with an unstripped binary and a DWP file
+3 - A stripped, split binary target with an --only-keep-debug symbols file and a DWP file
+"""
+
+
+class DebugInfodDWPTests(TestBase):
+ # No need to try every flavor of debug inf.
+ NO_DEBUG_INFO_TESTCASE = True
+
+ def setUp(self):
+ TestBase.setUp(self)
+ # Don't run these tests if we don't have Debuginfod support
+ if "Debuginfod" not in configuration.enabled_plugins:
+ self.skipTest("The Debuginfod SymbolLocator plugin is not enabled")
+
+ def test_normal_stripped(self):
+ """
+ Validate behavior with a stripped binary, no symbols or symbol locator.
+ """
+ self.config_test(["a.out"])
+ self.try_breakpoint(False)
+
+ def test_normal_stripped_split_with_dwp(self):
+ """
+ Validate behavior with symbols, but no symbol locator.
+ """
+ self.config_test(["a.out", "a.out.debug", "a.out.dwp"])
+ self.try_breakpoint(True)
+
+ def test_normal_stripped_only_dwp(self):
+ """
+ Validate behavior *with* dwp symbols only, but missing other symbols,
+ but no symbol locator. This shouldn't work: without the other symbols
+ DWO's appear mostly useless.
+ """
+ self.config_test(["a.out", "a.out.dwp"])
+ self.try_breakpoint(False)
+
+ def test_debuginfod_dwp_from_service(self):
+ """
+ Test behavior with the unstripped binary, and DWP from the service.
+ """
+ self.config_test(["a.out.debug"], "a.out.dwp")
+ self.try_breakpoint(True)
+
+ def test_debuginfod_both_symfiles_from_service(self):
+ """
+ Test behavior with a stripped binary, with the unstripped binary and
+ dwp symbols from Debuginfod.
+ """
+ self.config_test(["a.out"], "a.out.dwp", "a.out.unstripped")
+ self.try_breakpoint(True)
+
+ def test_debuginfod_both_okd_symfiles_from_service(self):
+ """
+ Test behavior with both the only-keep-debug symbols an...
[truncated]
|
|
Can someone please approve this, so I can yet again see if one of the weird buildbot configurations I don't have the ability to try out fails on these tests? The C++ code really ought to ship, as it fixes some egregious issues that were regressed several months ago (due to lack of tests...) |
JDevlieghere
approved these changes
May 1, 2024
nico
reviewed
May 2, 2024
| @@ -44,6 +44,10 @@ lldb_build_intel_pt = '@LLDB_BUILD_INTEL_PT@' | |||
| if lldb_build_intel_pt == '1': | |||
| config.enabled_plugins.append('intel-pt') | |||
|
|
|||
| llvm_enable_curl = '@LLVM_ENABLE_CURL@' | |||
There was a problem hiding this comment.
Is this correct? Won't this expand to 'NO' which is truthy when expanded with it off? Maybe you want this without quotes?
(Looks ike LLDB_BUILD_INTEL_PT gets this wrong too.)
There was a problem hiding this comment.
I validated that this works correctly (had the same work). The reason it works is because I added LLVM_ENABLE_CURL to the list in llvm_canonicalize_cmake_booleans found in lldb/test/CMakeLists.txt.
There was a problem hiding this comment.
Emulating the curses dependency approach would probably be more consistent (grep for skipIfCursesSupportMissing). Intel-pt does something different because it lives outside of (lib)lldb.
|
I am starting to see LLDB test failures in our toolchain builders after this patch: |
labath
added a commit
to labath/llvm-project
that referenced
this pull request
May 3, 2024
`ifeq` needs to be at the beginning of a line, otherwise it's interpreted as part of the recipe.
|
f8fedfb ought to fix |
labath
reviewed
May 3, 2024
|
|
||
| def getUUID(self, filename): | ||
| try: | ||
| target = self.dbg.CreateTarget(self.getBuildArtifact(filename)) |
There was a problem hiding this comment.
You don't actually need to create a target for this. Something like:
spec = lldb.SBModuleSpec()
spec.SetFileSpec(self.getBuildArtifact(filename))
lldb.SBModule(spec).GetUUIDString()
ought to suffice.
There was a problem hiding this comment.
I think this check is being too strict (and could be the cause of the reported failures). There's no requirement that the build id be exactly 10 bytes long, and in fact different linkers will use different build id lengths (corresponding to different checksum algorithms).
If all you wanted was to rule out lldb's fake crc-based UUIDs, then I suggest something like len(uuid) > 8.
If you absolutely must have a 10 byte uuid, you need to pass --build-id=sha1 to the linker.
| @@ -44,6 +44,10 @@ lldb_build_intel_pt = '@LLDB_BUILD_INTEL_PT@' | |||
| if lldb_build_intel_pt == '1': | |||
| config.enabled_plugins.append('intel-pt') | |||
|
|
|||
| llvm_enable_curl = '@LLVM_ENABLE_CURL@' | |||
There was a problem hiding this comment.
Emulating the curses dependency approach would probably be more consistent (grep for skipIfCursesSupportMissing). Intel-pt does something different because it lives outside of (lib)lldb.
DavidSpickett
added a commit
that referenced
this pull request
May 3, 2024
And "LLDB Debuginfod tests and a fix or two (#90622)". f8fedfb / 2d4acb0 As it has caused a test failure on 32 bit Arm: https://lab.llvm.org/buildbot/#/builders/17/builds/52580 Expr/TestStringLiteralExpr.test. The follow up did fix lang/c/shared_lib_stripped_symbols/TestSharedLibStrippedSymbols.py but not the other failure.
| if (!unstripped_spec) | ||
| return nullptr; | ||
| dsym_fspec = unstripped_spec.GetFileSpec(); | ||
| } |
There was a problem hiding this comment.
Something about this if block has broken Expr/TestStringLiteralExpr.test on Arm 32 bit, a platform that has been sensitive to changes in this area in the past. The test is built without debug info and something happens when the test file comes through here. For ld.so and the libc it's all the same as before.
(lldb) expr "hello there"
lldb ProcessGDBRemote::DoAllocateMemory no direct stub support for memory allocation, and InferiorCallMmap also failed - is stub missing register context save/restore capability?
Last time this happened it was because we lost the symbols that let us call mmap, but those are in ld.so usually so I'm not sure what's the problem this time.
This means we fall back the interpreter, which we didn't need to do before.
error: Can't evaluate the expression without a running target due to: Interpreter doesn't handle one of the expression's operands
I've reverted this PR (327bfc9) and the follow up while I investigate locally.
There was a problem hiding this comment.
We try to allocate memory to JIT the expression and calling mmap fails with a signal. In #68987 this was because we had lost the section info that told us whether to call it as Thumb or Arm, if we get that wrong it causes a SIGILL, so it could be the same thing again.
I will look into it more next week and assuming I find a fix, reland the changes for you.
There was a problem hiding this comment.
@DavidSpickett if there's anything I can do to help, please ping me. Feel free to use my github handle at hotmail to ping me, as I'm going to be on a "doing nothing in nicer weather" vacation next week.
There was a problem hiding this comment.
This is what I've figured out so far. It's like the previous issue I mentioned, except it's related to the program file, not ld.so.
If we look at the memory regions before this PR we see:
[0x00000000f7fe2000-0x00000000f7fe9000) ---
[0x00000000f7fe9000-0x00000000f7fea000) rw-
[0x00000000f7fea000-0x00000000f7feb000) r-- objc_imageinfo
[0x00000000f7feb000-0x00000000f7fec000) r-x .text
After we just have:
[0x00000000f7fe2000-0x00000000f7fec000) ---
Which means we have lost, or never loaded, the section information for this program file. Which makes sense given that it's compiled without debug information. We'll ask a plugin for the debug info and none of those will have it, so we exit early.
Problem is, Arm has 2 execution modes. Arm and Thumb. To know how to break properly in different code we look at markers on the sections. These set the AddressClass of the address value in LLDB to the right value.
In this case, we need to call mmap. This is in a Thumb section in ld.so, and we have it's section information. We don't need to break here though, only set the control register (cpsr) bit to indicate we want to run in Thumb mode, then write the PC to point to this location.
For the end of mmap, we need to return somewhere. So lldb says, why not _start, it's something we can count on existing on Linux. So LLDB needs to set the link register to that address, then place a breakpoint on the address to catch the program just as mmap finishes.
Problem is, after this PR we don't have section information to tell us that this area is Thumb. This means that Platform::GetSoftwareBreakpointTrapOpcode chooses an Arm breakpoint code. Now I don't know exactly what goes wrong there, I think in Thumb mode the program sees this encoding as some kind of backwards branch, and it ends up basically in the zero page and segfaults from there. This is why the call to mmap fails and we fall back to the interpreter.
So ideally we want to split the two concepts of 1. Section information and 2. Symbol file. So that we can load both without resetting the other. I'm going to look into that next.
And for some context, I think Arm is the only platform this section information is crucial. Potentially MIPS but also we dropped Linux MIPS support, and I don't know if those platforms mixed modes ever. So it's not surprising you didn't see this testing on more modern platforms.
There was a problem hiding this comment.
#91585 fixes the underlying issue, if/when that's proven to work, I'll put this PR back in.
There was a problem hiding this comment.
That PR is in, so at least Arm Linux is happy for this to go back in.
DavidSpickett
added a commit
to DavidSpickett/llvm-project
that referenced
this pull request
May 9, 2024
Instead of updating the member of the ObjectFileELF instance. This means that if one object file asks another to parse the symbol table, that first object's address class map is not left empty, which would break Arm and MIPS targets that need this information. This will fix the code added in llvm#90622 which broke the test `Expr/TestStringLiteralExpr.test` on 32 bit Arm Linux. This happened because we had the program file, then asked for a better object file, which returned the same program file again. This creates a second ObjectFileELF for the same file, so when we tell the second instance to parse the symbol table it actually calls into the first instance, leaving the address class map of the second instance empty. Which caused us to put an Arm breakpoint instuction at a Thumb return address and broke the ability to call mmap.
DavidSpickett
added a commit
to DavidSpickett/llvm-project
that referenced
this pull request
May 9, 2024
Code in llvm#90622 exposed a situation where a symbol table may be loaded via a second copy of the same object. This resulted in the first copy having its address class map updated, but not the second. And it was that second one we used for symbol lookups, since it was found by a plugin and we assume those to be more specific. (the plugins returning the same file may itself be a bug) I tried fixing this by returning the address map changes from the symbol table parsing. Which works but it's awkward to make sure both object's maps get updated. Instead, this change moves the address class map into the symbol table, which is shared between the objects already. ObjectFileELF::GetAddressClass was already checking whether the symbol table existed anyway, so we are ok to use it.
DavidSpickett
added a commit
that referenced
this pull request
May 10, 2024
…g methods (#91585) Instead of updating the member of the ObjectFileELF instance. This means that if one object file asks another to parse the symbol table, that first object's can update its address class map with the same changes that the other object did. (I'm not returning a reference to the other object's m_address_class_map member because there may be other things in there not related to the symbol table being parsed) This will fix the code added in #90622 which broke the test `Expr/TestStringLiteralExpr.test` on 32 bit Arm Linux. This happened because we had the program file, then asked for a better object file, which returned the same program file again. This creates a second ObjectFileELF for the same file, so when we tell the second instance to parse the symbol table it actually calls into the first instance, leaving the address class map of the second instance empty. Which caused us to put an Arm breakpoint instuction at a Thumb return address and broke the ability to call mmap.
|
I've fixed the underlying issue on Arm, you do not need to make any changes to this code to fix it. It should "just work" now. I see some unresolved comments about other test failures, and if I reland this I'll get all the buildbot emails and have no clue what to do to fix them. So instead I think it's best if you go over those, make any changes you need to and post a new PR to reland this. |
|
Thanks for the diagnostics & fix. I really appreciate it. I'm on vacation this week, so I'll get this ready & relanded next Monday. |
clayborg
pushed a commit
that referenced
this pull request
May 22, 2024
Here we go with attempt number five. Again, no changes to the LLDB code diff, which has been reviewed several times. For the tests, I added a `@skipIfCurlSupportMissing` annotation so that the Debuginfod mocked server stuff won't run, and I also disabled non-Linux/FreeBSD hosts altogether, as they fail for platform reasons on macOS and Windows. In addition, I updated the process for extracting the GNU BuildID to no create a target, per some feedback on the previous diff. For reference, previous PR's (landed, backed out after the fact for various reasons) #90622, #87676, #86812, #85693 --------- Co-authored-by: Kevin Frei <[email protected]>
clayborg
pushed a commit
that referenced
this pull request
Jul 8, 2024
This is the same diff I've put up at many times before. I've been trying to add some brand new functionality to the LLDB test infrastucture (create split-dwarf files!), and we all know that no good deed goes unpunished. The last attempt was reverted because it didn't work on the Fuchsia build. There are no code differences between this and [the](#90622) [previous](#87676) [four](#86812) [diffs](#85693) landed & reverted (due to testing infra failures). The only change in this one is the way `dwp` is being identified in `Makefile.rules`. Thanks to @petrhosek for helping me figure out how the fuchsia builders are configured. I now prefer to use llvm-dwp and fall back to gnu's dwp if the former isn't found. Hopefully this will work everywhere it needs to.
JDevlieghere
pushed a commit
that referenced
this pull request
Jul 10, 2024
This is all the tests and fixes I've had percolating since my first attempt at this in January. After 6 months of trying, I've given up on adding the ability to test DWP files in LLDB API tests. I've left both the tests (disabled) and the changes to Makefile.rules in place, in the hopes that someone who can configure the build bots will be able to enable the tests once a non-borked dwp tool is widely available. Other than disabling the DWP tests, this continues to be the same diff that I've tried to land and [not](#90622) [revert](#87676) [five](#86812) [times](#85693) [before](#96802). There are a couple of fixes that the testing exposed, and I've abandoned the DWP tests because I want to get those fixes finally upstreamed, as without them DebugInfoD is less useful.
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
This is the same diff I've put up at many times before. I've been trying to add some brand new functionality to the LLDB test infrastucture (create split-dwarf files!), and we all know that no good deed goes unpunished. The last attempt was reverted because it didn't work on the Fuchsia build. There are no code differences between this and [the](llvm#90622) [previous](llvm#87676) [four](llvm#86812) [diffs](llvm#85693) landed & reverted (due to testing infra failures). The only change in this one is the way `dwp` is being identified in `Makefile.rules`. Thanks to @petrhosek for helping me figure out how the fuchsia builders are configured. I now prefer to use llvm-dwp and fall back to gnu's dwp if the former isn't found. Hopefully this will work everywhere it needs to.
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
…#98344) This is all the tests and fixes I've had percolating since my first attempt at this in January. After 6 months of trying, I've given up on adding the ability to test DWP files in LLDB API tests. I've left both the tests (disabled) and the changes to Makefile.rules in place, in the hopes that someone who can configure the build bots will be able to enable the tests once a non-borked dwp tool is widely available. Other than disabling the DWP tests, this continues to be the same diff that I've tried to land and [not](llvm#90622) [revert](llvm#87676) [five](llvm#86812) [times](llvm#85693) [before](llvm#96802). There are a couple of fixes that the testing exposed, and I've abandoned the DWP tests because I want to get those fixes finally upstreamed, as without them DebugInfoD is less useful.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm taking yet another swing at getting these tests going, on the hypothesis that the problems with buildbots & whatnot are because they're not configured with CURL support, which I've confirmed would cause the previous tests to fail. (I have no access to an ARM64 linux system, but I did repro the failure on MacOS configured without CURL support)
So, the only difference between this diff and previous diffs that have already been approved is that I've added a condition to the tests to only run if Debuginfod capabilities should be built into the binary. I had done this for these tests when they were Shell tests and not API tests, but I couldn't find a direct analog in any API test, so I used the "plugins" model used by the intel-pt tests as well.