https://github.com/dmpots created https://github.com/llvm/llvm-project/pull/133826
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. >From 6a6e5e98860112cf50e83059dbad94bdc9dd4cc2 Mon Sep 17 00:00:00 2001 From: David Peixotto <p...@meta.com> Date: Mon, 31 Mar 2025 13:14:49 -0700 Subject: [PATCH] [lldb] Fix intel trace plugin tests 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. --- .../test/tools/intelpt/intelpt_testcase.py | 7 +++ lldb/source/Target/ProcessTrace.cpp | 3 +- .../trace/TestTraceDumpFunctionCalls.py | 2 + .../API/commands/trace/TestTraceEvents.py | 1 + lldb/test/API/commands/trace/TestTraceSave.py | 2 + .../API/commands/trace/TestTraceStartStop.py | 1 + lldb/test/API/commands/trace/TestTraceTSC.py | 1 + .../TestTraceStartStopMultipleThreads.py | 1 + lldb/unittests/Process/CMakeLists.txt | 4 +- lldb/unittests/Process/ProcessTraceTest.cpp | 63 +++++++++++++++++++ 10 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 lldb/unittests/Process/ProcessTraceTest.cpp 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); +} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
