Skip to content

Commit

Permalink
DebugInfoD issues, take 2 (#86812)
Browse files Browse the repository at this point in the history
The previous diff (and it's subsequent fix) were reverted as the tests
didn't work properly on the AArch64 & ARM LLDB buildbots. I made a
couple more minor changes to tests (from @clayborg's feedback) and
disabled them for non Linux-x86(_64) builds, as I don't have the ability
do anything about an ARM64 Linux failure. If I had to guess, I'd say the
toolchain on the buildbots isn't respecting the `-Wl,--build-id` flag.
Maybe, one day, when I have a Linux AArch64 system I'll dig in to it.

From the reverted PR:

I've migrated the tests in my
#79181 from shell to API (at
@JDevlieghere's suggestion) and addressed a couple issues that were
exposed during testing.

The tests first test the "normal" situation (no DebugInfoD involvement,
just normal debug files sitting around), then the "no debug info"
situation (to make sure the test is seeing failure properly), then it
tests to validate that when DebugInfoD returns the symbols, things work
properly. This is duplicated for DWP/split-dwarf scenarios.

---------

Co-authored-by: Kevin Frei <freik@meta.com>
  • Loading branch information
kevinfrei and kevinfrei committed Apr 3, 2024
1 parent e29228e commit a8425d2
Show file tree
Hide file tree
Showing 10 changed files with 506 additions and 17 deletions.
26 changes: 25 additions & 1 deletion lldb/packages/Python/lldbsuite/test/make/Makefile.rules
Original file line number Diff line number Diff line change
Expand Up @@ -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.
#----------------------------------------------------------------------
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

#----------------------------------------------------------------------
Expand Down
38 changes: 25 additions & 13 deletions lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
7 changes: 6 additions & 1 deletion lldb/source/Plugins/SymbolLocator/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -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)
29 changes: 27 additions & 2 deletions lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
19 changes: 19 additions & 0 deletions lldb/test/API/debuginfod/Normal/Makefile
Original file line number Diff line number Diff line change
@@ -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
179 changes: 179 additions & 0 deletions lldb/test/API/debuginfod/Normal/TestDebuginfod.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
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
"""


# It looks like Linux-AArch64 doesn't support build-id's on the LLDB builtbots
@skipIf(oslist=no_match(["linux"]), archs=no_match(["i386", "x86_64"]))
class DebugInfodTests(TestBase):
# No need to try every flavor of debug inf.
NO_DEBUG_INFO_TESTCASE = True

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
7 changes: 7 additions & 0 deletions lldb/test/API/debuginfod/Normal/main.c
Original file line number Diff line number Diff line change
@@ -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); }
Loading

0 comments on commit a8425d2

Please sign in to comment.