Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LLDB Debuginfod usage tests (with fixes) #79181

Closed
wants to merge 16 commits into from

Conversation

kevinfrei
Copy link
Contributor

@kevinfrei kevinfrei commented Jan 23, 2024

This is a collection of tests for Debuginfod usage inside LLDB, with a fix or two for a couple issues that the tests exposed. I've included documentation for what the tests do as a markdown file that lives with the tests.

Copy link

github-actions bot commented Jan 23, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 3714f937b835c06c8c32ca4f3f61ba2317db2296 ea496707c03a093a70a746254a3ed81e2d4c3d78 -- lldb/test/API/debuginfod/Normal/main.c lldb/test/API/debuginfod/SplitDWARF/main.c lldb/test/Shell/Debuginfod/Inputs/main.c lldb/source/Plugins/SymbolVendor/ELF/SymbolVendorELF.cpp
View the diff from clang-format here.
diff --git a/lldb/test/API/debuginfod/Normal/main.c b/lldb/test/API/debuginfod/Normal/main.c
index 2cc7e1557f..4c7184609b 100644
--- a/lldb/test/API/debuginfod/Normal/main.c
+++ b/lldb/test/API/debuginfod/Normal/main.c
@@ -4,6 +4,4 @@ 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);
-}
+int main(int argc, const char *argv[]) { return func(0, argv); }
diff --git a/lldb/test/API/debuginfod/SplitDWARF/main.c b/lldb/test/API/debuginfod/SplitDWARF/main.c
index 2cc7e1557f..4c7184609b 100644
--- a/lldb/test/API/debuginfod/SplitDWARF/main.c
+++ b/lldb/test/API/debuginfod/SplitDWARF/main.c
@@ -4,6 +4,4 @@ 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);
-}
+int main(int argc, const char *argv[]) { return func(0, argv); }
diff --git a/lldb/test/Shell/Debuginfod/Inputs/main.c b/lldb/test/Shell/Debuginfod/Inputs/main.c
index 584eb11072..3a836b363c 100644
--- a/lldb/test/Shell/Debuginfod/Inputs/main.c
+++ b/lldb/test/Shell/Debuginfod/Inputs/main.c
@@ -1,10 +1,6 @@
 // A script to (re)create the .yaml files is in 'make-inputs'. If you make changes
 // you'll need to update the .note.gnu.buildid values in the tests, as the cache names
 
-int func(int argc, const char **argv) {
-  return (argc + 1) * (argv[argc][0] + 2);
-}
+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);
-}
+int main(int argc, const char *argv[]) { return func(0, argv); }

@kevinfrei kevinfrei force-pushed the debuginfod-test-and-fix branch from 28da9b5 to e74b2a2 Compare January 23, 2024 22:35
@kevinfrei kevinfrei marked this pull request as ready for review January 23, 2024 22:38
Copy link
Member

@JDevlieghere JDevlieghere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if shell test are really the best way to test this. For more complex scenarios like are being tested here, we generally prefer [1] API tests because they're more expressive and allow you to build more complicated test binaries with our Makefile system. Things like stripping a binary and generating split DWARF all seems like things that could be done that way, to avoid checking in a (yaml-ized) binary.

Did you consider/evaluate writing API tests for this?

[1] https://lldb.llvm.org/resources/test.html

lldb/test/Shell/Debuginfod/Debuginfod-testing.md Outdated Show resolved Hide resolved
@kevinfrei
Copy link
Contributor Author

I'm wondering if shell test are really the best way to test this. For more complex scenarios like are being tested here, we generally prefer [1] API tests because they're more expressive and allow you to build more complicated test binaries with our Makefile system. Things like stripping a binary and generating split DWARF all seems like things that could be done that way, to avoid checking in a (yaml-ized) binary.

Did you consider/evaluate writing API tests for this?

[1] https://lldb.llvm.org/resources/test.html

The next addition (async support) requires API tests, so I figured getting some shell tests in place to do a more "user experience" kind of testing was reasonable, since there will be API-based tests coming. If you feel like moving this stuff to API tests wholesale is the right thing to do, I'm happy to go figure that out next week.

@kevinfrei kevinfrei marked this pull request as draft February 16, 2024 16:55
@kevinfrei kevinfrei force-pushed the debuginfod-test-and-fix branch 4 times, most recently from 570639a to 526ee41 Compare February 22, 2024 19:20
@kevinfrei kevinfrei force-pushed the debuginfod-test-and-fix branch 2 times, most recently from a54dd84 to d13cb6f Compare March 4, 2024 23:49
Copy link

github-actions bot commented Mar 7, 2024

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:
darker --check --diff -r 3714f937b835c06c8c32ca4f3f61ba2317db2296...ea496707c03a093a70a746254a3ed81e2d4c3d78 lldb/test/API/debuginfod/Normal/TestDebuginfod.py lldb/test/API/debuginfod/SplitDWARF/TestDebuginfodDWP.py lldb/test/Shell/lit.cfg.py
View the diff from darker here.
--- API/debuginfod/Normal/TestDebuginfod.py	2024-03-07 17:36:44.000000 +0000
+++ API/debuginfod/Normal/TestDebuginfod.py	2024-03-07 17:39:39.339233 +0000
@@ -4,27 +4,30 @@
 
 import lldb
 import lldbsuite.test.lldbutil as lldbutil
 from lldbsuite.test.lldbtest import *
 
+
 def getUUID(aoutuuid):
     """
     Pull the 20 byte UUID out of the .note.gnu.build-id section that was dumped
     to a file already, as part of the build.
     """
     import struct
+
     with open(aoutuuid, "rb") as f:
         data = f.read(36)
         if len(data) != 36:
             return None
         header = struct.unpack_from("<4I", data)
         if len(header) != 4:
             return None
         # 4 element 'prefix', 20 bytes of uuid, 3 byte long string, 'GNU':
-        if header[0] != 4 or header[1] != 20 or header[2] != 3 or header[3] != 0x554e47:
+        if header[0] != 4 or header[1] != 20 or header[2] != 3 or header[3] != 0x554E47:
             return None
         return data[16:].hex()
+
 
 def config_test(local_files, uuid, debuginfo, executable):
     """
     Set up a test with local_files[] copied to a particular location
     so that we control which files are, or are not, found in the file system.
@@ -72,10 +75,11 @@
 # 2 - A stripped binary with a corresponding --only-keep-debug symbols file
 # 3 - A split binary with it's corresponding DWP file
 # 4 - A stripped, split binary with an unstripped binary and a DWP file
 # 5 - A stripped, split binary with an --only-keep-debug symbols file and a DWP file
 
+
 class DebugInfodTests(TestBase):
     # No need to try every flavor of debug inf.
     NO_DEBUG_INFO_TESTCASE = True
 
     def test_stuff(self):
@@ -90,13 +94,19 @@
         # Setup the fake DebugInfoD server.
         server_root = config_fake_debuginfod_server(self.uuid, self.dwp, self.debugbin)
 
         # Configure LLDB properly
         self.runCmd("settings set symbols.enable-external-lookup true")
-        self.runCmd("settings set plugin.symbol-locator.debuginfod.cache-path %s/cache" % server_root)
+        self.runCmd(
+            "settings set plugin.symbol-locator.debuginfod.cache-path %s/cache"
+            % server_root
+        )
         self.runCmd("settings clear plugin.symbol-locator.debuginfod.server-urls")
-        cmd = "settings insert-before plugin.symbol-locator.debuginfod.server-urls 0 file://%s" % server_root
+        cmd = (
+            "settings insert-before plugin.symbol-locator.debuginfod.server-urls 0 file://%s"
+            % server_root
+        )
         self.runCmd(cmd)
         print(cmd)
         # Check to see if the symbol file is properly loaded
         self.main_source_file = lldb.SBFileSpec("main.c")
         self.sample_test()
@@ -119,11 +129,14 @@
         addr = loc.GetAddress()
         self.assertTrue(addr and addr.IsValid(), "Loc address is valid")
         line_entry = addr.GetLineEntry()
         self.assertTrue(line_entry and line_entry.IsValid(), "Loc line entry is valid")
         self.assertEqual(line_entry.GetLine(), 18)
-        self.assertEqual(loc.GetLineEntry().GetFileSpec().GetFilename(), self.main_source_file.GetFilename())
+        self.assertEqual(
+            loc.GetLineEntry().GetFileSpec().GetFilename(),
+            self.main_source_file.GetFilename(),
+        )
 
     def sample_test_no_launch(self):
         """Same as above but doesn't launch a process."""
 
         target = self.createTestTarget()
--- API/debuginfod/SplitDWARF/TestDebuginfodDWP.py	2024-03-07 17:36:44.000000 +0000
+++ API/debuginfod/SplitDWARF/TestDebuginfodDWP.py	2024-03-07 17:39:39.375680 +0000
@@ -4,27 +4,30 @@
 
 import lldb
 import lldbsuite.test.lldbutil as lldbutil
 from lldbsuite.test.lldbtest import *
 
+
 def getUUID(aoutuuid):
     """
     Pull the 20 byte UUID out of the .note.gnu.build-id section that was dumped
     to a file already, as part of the build.
     """
     import struct
+
     with open(aoutuuid, "rb") as f:
         data = f.read(36)
         if len(data) != 36:
             return None
         header = struct.unpack_from("<4I", data)
         if len(header) != 4:
             return None
         # 4 element 'prefix', 20 bytes of uuid, 3 byte long string, 'GNU':
-        if header[0] != 4 or header[1] != 20 or header[2] != 3 or header[3] != 0x554e47:
+        if header[0] != 4 or header[1] != 20 or header[2] != 3 or header[3] != 0x554E47:
             return None
         return data[16:].hex()
+
 
 def config_fake_debuginfod_server(uuid, debuginfo, executable):
     """
     Create a file-system 'hosted' debuginfod server for the given UUID.
     Make the filesystem look like:
@@ -35,32 +38,34 @@
     Returns the /tmp/<tmpdir> path
     """
     import os
     import shutil
     import tempfile
+
     tmpdir = tempfile.mkdtemp()
     uuiddir = os.path.join(tmpdir, "buildid", uuid)
     os.makedirs(uuiddir)
     os.makedirs(os.path.join(tmpdir, "cache"))
     # Move the files to the correct location for debuginfod to find
     if debuginfo:
         pass
-        #shutil.move(debuginfo, os.path.join(uuiddir, "debuginfo"))
+        # shutil.move(debuginfo, os.path.join(uuiddir, "debuginfo"))
 
     if executable:
         pass
-        #shutil.move(executable, os.path.join(uuiddir, "executable"))
+        # shutil.move(executable, os.path.join(uuiddir, "executable"))
 
     return tmpdir
 
 
 # Need to test 5 different scenarios:
 # 1 - A stripped binary with it's corresponding unstripped binary:
 # 2 - A stripped binary with a corresponding --only-keep-debug symbols file
 # 3 - A split binary with it's corresponding DWP file
 # 4 - A stripped, split binary with an unstripped binary and a DWP file
 # 5 - A stripped, split binary with an --only-keep-debug symbols file and a DWP file
+
 
 class DebugInfodTests(TestBase):
     # No need to try every flavor of debug inf.
     NO_DEBUG_INFO_TESTCASE = True
 
@@ -76,13 +81,19 @@
         # Setup the fake DebugInfoD server.
         server_root = config_fake_debuginfod_server(self.uuid, self.dwp, self.debugbin)
 
         # Configure LLDB properly
         self.runCmd("settings set symbols.enable-external-lookup true")
-        self.runCmd("settings set plugin.symbol-locator.debuginfod.cache-path %s/cache" % server_root)
+        self.runCmd(
+            "settings set plugin.symbol-locator.debuginfod.cache-path %s/cache"
+            % server_root
+        )
         self.runCmd("settings clear plugin.symbol-locator.debuginfod.server-urls")
-        cmd = "settings insert-before plugin.symbol-locator.debuginfod.server-urls 0 file://%s" % server_root
+        cmd = (
+            "settings insert-before plugin.symbol-locator.debuginfod.server-urls 0 file://%s"
+            % server_root
+        )
         self.runCmd(cmd)
         print(cmd)
         # Check to see if the symbol file is properly loaded
         self.main_source_file = lldb.SBFileSpec("main.c")
         self.sample_test()
@@ -105,11 +116,14 @@
         addr = loc.GetAddress()
         self.assertTrue(addr and addr.IsValid(), "Loc address is valid")
         line_entry = addr.GetLineEntry()
         self.assertTrue(line_entry and line_entry.IsValid(), "Loc line entry is valid")
         self.assertEqual(line_entry.GetLine(), 18)
-        self.assertEqual(loc.GetLineEntry().GetFileSpec().GetFilename(), self.main_source_file.GetFilename())
+        self.assertEqual(
+            loc.GetLineEntry().GetFileSpec().GetFilename(),
+            self.main_source_file.GetFilename(),
+        )
 
     def sample_test_no_launch(self):
         """Same as above but doesn't launch a process."""
 
         target = self.createTestTarget()

@kevinfrei kevinfrei force-pushed the debuginfod-test-and-fix branch from 2c6afa8 to 1535a16 Compare March 7, 2024 16:21
@kevinfrei kevinfrei force-pushed the debuginfod-test-and-fix branch from 1535a16 to ea49670 Compare March 7, 2024 17:37
@kevinfrei kevinfrei closed this Mar 19, 2024
clayborg pushed a commit that referenced this pull request Mar 21, 2024
Finally getting back to Debuginfod tests:
I've migrated the tests in my [earlier
PR](#79181) from shell to API
(at @JDevlieghere's suggestion) and addressed a couple issues that came
about 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 <[email protected]>
@kevinfrei kevinfrei deleted the debuginfod-test-and-fix branch March 21, 2024 20:28
chencha3 pushed a commit to chencha3/llvm-project that referenced this pull request Mar 23, 2024
Finally getting back to Debuginfod tests:
I've migrated the tests in my [earlier
PR](llvm#79181) from shell to API
(at @JDevlieghere's suggestion) and addressed a couple issues that came
about 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 <[email protected]>
clayborg pushed a commit that referenced this pull request Apr 3, 2024
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 <[email protected]>
clayborg pushed a commit that referenced this pull request Apr 4, 2024
I believe I've got the tests properly configured to only run on Linux
x86(_64), as I don't have a Linux AArch64/Arm device to diagnose what's
going wrong with the tests (I suspect there's some issue with generating
`.note.gnu.build-id` sections...)

The actual code fixes have now been reviewed 3 times:
#79181 (moved shell tests to
API tests), #85693 (Changed
some of the testing infra), and
#86812 (didn't get the tests
configured quite right). The Debuginfod integration for symbol
acquisition in LLDB now works with the `executable` and `debuginfo`
Debuginfod network requests working properly for normal, `objcopy
--only-keep-debug` stripped, split-dwarf, and `objcopy
--only-keep-debug` stripped *plus* split-dwarf symbols/binaries.

The reasons for the multiple attempts have been tests on platforms I
don't have access to (Linux AArch64/Arm + MacOS x86_64). I believe I've
got the tests properly disabled for everything except for Linux x86(_64)
now. I've built & tested on MacOS AArch64 and Linux x86_64.

---------

Co-authored-by: Kevin Frei <[email protected]>
clayborg pushed a commit that referenced this pull request May 2, 2024
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](#85693)
[diffs](#87676) 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](#79181) 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.

---------

Co-authored-by: Kevin Frei <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants