[lldb] Fix intel trace plugin tests #133826
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
The tests for the [intel-pt](https://github.com/llvm/llvm-project/blob/348374028970c956f2e49ab7553b495d7408ccd9/lldb/docs/use/intel_pt.rst) trace plugin were failing for multiple reasons. On machines where tracing is supported many of the tests were crashing because of a nullptr dereference. It looks like the `core_file` parameter in `ProcessTrace::CreateInstance` was once ignored, but was changed to always being dereferenced. This caused the tests to fail even when tracing was supported. On machines where tracing is not supported we would still run tests that attempt to take a trace. These would obviously fail because the required hardware is not present. Note that some of the tests simply read serialized json as trace files which does not require any special hardware. This PR fixes these two issues by guarding the pointer dereference and then skipping unsupported tests on machines. With these changes the trace tests pass on both types of machines. We also add a new unit test to validate that a process can be created with a nullptr core_file through the generic process trace plugin path.
|
@llvm/pr-subscribers-lldb Author: David Peixotto (dmpots) ChangesThe tests for the On machines where tracing is supported many of the tests were crashing because of a nullptr dereference. It looks like the On machines where tracing is not supported we would still run tests that attempt to take a trace. These would obviously fail because the required hardware is not present. Note that some of the tests simply read serialized json as trace files which does not require any special hardware. This PR fixes these two issues by guarding the pointer dereference and then skipping unsupported tests on machines. With these changes the trace tests pass on both types of machines. We also add a new unit test to validate that a process can be created with a nullptr core_file through the generic process trace plugin path. Full diff: https://github.com/llvm/llvm-project/pull/133826.diff 10 Files Affected:
diff --git a/lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py b/lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
index f1b7d7c33bf07..176dce6a435f7 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
@@ -18,6 +18,13 @@ def wrapper(*args, **kwargs):
return wrapper
+def skipIfCpuDoesNotSupportIntelPT(func):
+ """Skip tests if the system does not support tracing."""
+
+ supported = os.path.exists("/sys/bus/event_source/devices/intel_pt/type")
+ return unittest.skipIf(not supported, "intel-pt tracing is unsupported")(func)
+
+
# Class that should be used by all python Intel PT tests.
#
# It has a handy check that skips the test if the intel-pt plugin is not enabled.
diff --git a/lldb/source/Target/ProcessTrace.cpp b/lldb/source/Target/ProcessTrace.cpp
index f131339905474..02272b1651da5 100644
--- a/lldb/source/Target/ProcessTrace.cpp
+++ b/lldb/source/Target/ProcessTrace.cpp
@@ -36,7 +36,8 @@ ProcessSP ProcessTrace::CreateInstance(TargetSP target_sp,
bool can_connect) {
if (can_connect)
return nullptr;
- return std::make_shared<ProcessTrace>(target_sp, listener_sp, *crash_file);
+ return std::make_shared<ProcessTrace>(target_sp, listener_sp,
+ crash_file ? *crash_file : FileSpec());
}
bool ProcessTrace::CanDebug(TargetSP target_sp, bool plugin_specified_by_name) {
diff --git a/lldb/test/API/commands/trace/TestTraceDumpFunctionCalls.py b/lldb/test/API/commands/trace/TestTraceDumpFunctionCalls.py
index 761c262ae4de0..8b30414953d7d 100644
--- a/lldb/test/API/commands/trace/TestTraceDumpFunctionCalls.py
+++ b/lldb/test/API/commands/trace/TestTraceDumpFunctionCalls.py
@@ -133,6 +133,7 @@ def testFunctionCallsWithErrors(self):
],
)
+ @skipIfCpuDoesNotSupportIntelPT
def testInlineFunctionCalls(self):
self.expect(
"file " + os.path.join(self.getSourceDir(), "inline-function", "a.out")
@@ -194,6 +195,7 @@ def testInlineFunctionCalls(self):
],
)
+ @skipIfCpuDoesNotSupportIntelPT
def testIncompleteInlineFunctionCalls(self):
self.expect(
"file " + os.path.join(self.getSourceDir(), "inline-function", "a.out")
diff --git a/lldb/test/API/commands/trace/TestTraceEvents.py b/lldb/test/API/commands/trace/TestTraceEvents.py
index c20bcc247105b..7e6e07db0c1dd 100644
--- a/lldb/test/API/commands/trace/TestTraceEvents.py
+++ b/lldb/test/API/commands/trace/TestTraceEvents.py
@@ -45,6 +45,7 @@ def testCPUEvents(self):
],
)
+ @skipIfCpuDoesNotSupportIntelPT
@testSBAPIAndCommands
def testPauseEvents(self):
"""
diff --git a/lldb/test/API/commands/trace/TestTraceSave.py b/lldb/test/API/commands/trace/TestTraceSave.py
index af38669cb4fce..d8f39a1f3fa2a 100644
--- a/lldb/test/API/commands/trace/TestTraceSave.py
+++ b/lldb/test/API/commands/trace/TestTraceSave.py
@@ -43,6 +43,7 @@ def testErrorMessages(self):
"trace save", substrs=["error: Process is not being traced"], error=True
)
+ @skipIfCpuDoesNotSupportIntelPT
def testSaveToInvalidDir(self):
self.expect(
"target create "
@@ -165,6 +166,7 @@ def checkSessionBundle(session_file_path):
copied_cpu = find(lambda cor: cor["id"] == cpu["id"], copy["cpus"])
self.assertIsNotNone(copied_cpu)
+ @skipIfCpuDoesNotSupportIntelPT
def testSaveTrace(self):
self.expect(
"target create "
diff --git a/lldb/test/API/commands/trace/TestTraceStartStop.py b/lldb/test/API/commands/trace/TestTraceStartStop.py
index 5add321b4c83f..94b460bf1623a 100644
--- a/lldb/test/API/commands/trace/TestTraceStartStop.py
+++ b/lldb/test/API/commands/trace/TestTraceStartStop.py
@@ -5,6 +5,7 @@
from lldbsuite.test.decorators import *
+@skipIfCpuDoesNotSupportIntelPT
class TestTraceStartStop(TraceIntelPTTestCaseBase):
def expectGenericHelpMessageForStartCommand(self):
self.expect(
diff --git a/lldb/test/API/commands/trace/TestTraceTSC.py b/lldb/test/API/commands/trace/TestTraceTSC.py
index 4a19065e60c2b..2a412b197b704 100644
--- a/lldb/test/API/commands/trace/TestTraceTSC.py
+++ b/lldb/test/API/commands/trace/TestTraceTSC.py
@@ -5,6 +5,7 @@
from lldbsuite.test.decorators import *
+@skipIfCpuDoesNotSupportIntelPT
class TestTraceTimestampCounters(TraceIntelPTTestCaseBase):
@testSBAPIAndCommands
@skipIf(oslist=no_match(["linux"]), archs=no_match(["i386", "x86_64"]))
diff --git a/lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py b/lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
index 12f99f07c78a8..658a62d894cc8 100644
--- a/lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
+++ b/lldb/test/API/commands/trace/multiple-threads/TestTraceStartStopMultipleThreads.py
@@ -6,6 +6,7 @@
from lldbsuite.test.decorators import *
+@skipIfCpuDoesNotSupportIntelPT
class TestTraceStartStopMultipleThreads(TraceIntelPTTestCaseBase):
@skipIf(oslist=no_match(["linux"]), archs=no_match(["i386", "x86_64"]))
@testSBAPIAndCommands
diff --git a/lldb/unittests/Process/CMakeLists.txt b/lldb/unittests/Process/CMakeLists.txt
index d3b37e006fd89..a240d773c3f30 100644
--- a/lldb/unittests/Process/CMakeLists.txt
+++ b/lldb/unittests/Process/CMakeLists.txt
@@ -7,8 +7,9 @@ endif()
add_subdirectory(Utility)
add_subdirectory(minidump)
-add_lldb_unittest(ProcessEventDataTests
+add_lldb_unittest(ProcessTests
ProcessEventDataTest.cpp
+ ProcessTraceTest.cpp
LINK_LIBS
lldbCore
@@ -18,5 +19,6 @@ add_lldb_unittest(ProcessEventDataTests
lldbUtility
lldbUtilityHelpers
lldbInterpreter
+ lldbPluginPlatformLinux
lldbPluginPlatformMacOSX
)
diff --git a/lldb/unittests/Process/ProcessTraceTest.cpp b/lldb/unittests/Process/ProcessTraceTest.cpp
new file mode 100644
index 0000000000000..fc6b92e868248
--- /dev/null
+++ b/lldb/unittests/Process/ProcessTraceTest.cpp
@@ -0,0 +1,63 @@
+//===-- ProcessEventDataTest.cpp ------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Target/ProcessTrace.h"
+#include "Plugins/Platform/Linux/PlatformLinux.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Host/HostInfo.h"
+#include "gtest/gtest.h"
+
+using namespace lldb_private;
+using namespace lldb;
+using namespace platform_linux;
+
+// This is needed for the tests that create a trace process.
+class ProcessTraceTest : public ::testing::Test {
+public:
+ void SetUp() override {
+ ProcessTrace::Initialize();
+ FileSystem::Initialize();
+ HostInfo::Initialize();
+ PlatformLinux::Initialize();
+ }
+ void TearDown() override {
+ PlatformLinux::Initialize();
+ HostInfo::Terminate();
+ FileSystem::Terminate();
+ ProcessTrace::Terminate();
+ }
+};
+
+TargetSP CreateTarget(DebuggerSP &debugger_sp, const ArchSpec &arch) {
+ PlatformSP platform_sp;
+ TargetSP target_sp;
+ debugger_sp->GetTargetList().CreateTarget(
+ *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
+ return target_sp;
+}
+
+// Test that we can create a process trace with a nullptr core file.
+TEST_F(ProcessTraceTest, ConstructorWithNullptrCoreFile) {
+ ArchSpec arch("i386-pc-linux");
+
+ Platform::SetHostPlatform(PlatformLinux::CreateInstance(true, &arch));
+ ASSERT_NE(Platform::GetHostPlatform(), nullptr);
+
+ DebuggerSP debugger_sp = Debugger::CreateInstance();
+ ASSERT_TRUE(debugger_sp);
+
+ TargetSP target_sp = CreateTarget(debugger_sp, arch);
+ ASSERT_TRUE(target_sp);
+
+ ProcessSP process_sp = target_sp->CreateProcess(
+ /*listener*/ nullptr, "trace",
+ /*crash_file*/ nullptr,
+ /*can_connect*/ false);
+
+ ASSERT_NE(process_sp, nullptr);
+}
|
|
Here are the relevant test result changes for this change. Trace Hardware UnsupportedBeforeAfterTrace Hardware SupportedBeforeAfter |
JDevlieghere
approved these changes
Apr 1, 2025
lldb/packages/Python/lldbsuite/test/tools/intelpt/intelpt_testcase.py
Outdated
Show resolved
Hide resolved
skipIfCpuDoesNotSupportIntelPT -> skipIfNoIntelPT
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
Apr 2, 2025
The tests for the [intel-pt](https://github.com/llvm/llvm-project/blob/348374028970c956f2e49ab7553b495d7408ccd9/lldb/docs/use/intel_pt.rst) trace plugin were failing for multiple reasons. On machines where tracing is supported many of the tests were crashing because of a nullptr dereference. It looks like the `core_file` parameter in `ProcessTrace::CreateInstance` was once ignored, but was changed to always being dereferenced. This caused the tests to fail even when tracing was supported. On machines where tracing is not supported we would still run tests that attempt to take a trace. These would obviously fail because the required hardware is not present. Note that some of the tests simply read serialized json as trace files which does not require any special hardware. This PR fixes these two issues by guarding the pointer dereference and then skipping unsupported tests on machines. With these changes the trace tests pass on both types of machines. We also add a new unit test to validate that a process can be created with a nullptr core_file through the generic process trace plugin path.
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.
The tests for the
intel-pt trace plugin were failing for multiple reasons.
On machines where tracing is supported many of the tests were crashing because of a nullptr dereference. It looks like the
core_fileparameter inProcessTrace::CreateInstancewas once ignored, but was changed to always being dereferenced. This caused the tests to fail even when tracing was supported.On machines where tracing is not supported we would still run tests that attempt to take a trace. These would obviously fail because the required hardware is not present. Note that some of the tests simply read serialized json as trace files which does not require any special hardware.
This PR fixes these two issues by guarding the pointer dereference and then skipping unsupported tests on machines. With these changes the trace tests pass on both types of machines.
We also add a new unit test to validate that a process can be created with a nullptr core_file through the generic process trace plugin path.