Add a unit test for SBBreakpoint::SetCallback #96001

chelcassanova · 2024-06-18T22:20:44Z

This commit adds a unit test for SBBreakpoint::SetCallback as it wasn't being tested before.

llvmbot · 2024-06-18T22:21:13Z

@llvm/pr-subscribers-lldb

Author: Chelsea Cassanova (chelcassanova)

Changes

This commit adds a unit test for SBBreakpoint::SetCallback as it wasn't being tested before.

Full diff: https://github.com/llvm/llvm-project/pull/96001.diff

2 Files Affected:

(modified) lldb/unittests/Breakpoint/CMakeLists.txt (+4)
(added) lldb/unittests/Breakpoint/TestBreakpointSetCallback.cpp (+90)

diff --git a/lldb/unittests/Breakpoint/CMakeLists.txt b/lldb/unittests/Breakpoint/CMakeLists.txt
index 757c2da1a4d9d..858a2151c503b 100644
--- a/lldb/unittests/Breakpoint/CMakeLists.txt
+++ b/lldb/unittests/Breakpoint/CMakeLists.txt
@@ -1,10 +1,14 @@
 add_lldb_unittest(LLDBBreakpointTests
   BreakpointIDTest.cpp
   WatchpointAlgorithmsTests.cpp
+  TestBreakpointSetCallback.cpp
 
   LINK_LIBS
     lldbBreakpoint
     lldbCore
+    LLVMTestingSupport
+    lldbUtilityHelpers
+    lldbPluginPlatformMacOSX
   LINK_COMPONENTS
     Support
   )
diff --git a/lldb/unittests/Breakpoint/TestBreakpointSetCallback.cpp b/lldb/unittests/Breakpoint/TestBreakpointSetCallback.cpp
new file mode 100644
index 0000000000000..351384230f15f
--- /dev/null
+++ b/lldb/unittests/Breakpoint/TestBreakpointSetCallback.cpp
@@ -0,0 +1,90 @@
+//===-- TestBreakpointSetCallback.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 "Plugins/Platform/MacOSX/PlatformMacOSX.h"
+#include "Plugins/Platform/MacOSX/PlatformRemoteMacOSX.h"
+#include "TestingSupport/SubsystemRAII.h"
+#include "TestingSupport/TestUtilities.h"
+#include "lldb/Breakpoint/StoppointCallbackContext.h"
+#include "lldb/Core/Debugger.h"
+#include "lldb/Core/Progress.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/Host/HostInfo.h"
+#include "lldb/Target/ExecutionContext.h"
+#include "lldb/lldb-private-enumerations.h"
+#include "lldb/lldb-types.h"
+#include "gtest/gtest.h"
+#include <iostream>
+#include <memory>
+#include <mutex>
+
+using namespace lldb_private;
+using namespace lldb;
+
+class BreakpointSetCallbackTest : public ::testing::Test {
+public:
+  static void CheckCallbackArgs(void *baton, StoppointCallbackContext *context,
+                                lldb::user_id_t break_id,
+                                lldb::user_id_t break_loc_id,
+                                lldb::user_id_t expected_breakpoint_id,
+                                lldb::user_id_t expected_breakpoint_loc_id) {
+    std::cout << "HELLO" << std::endl;
+    EXPECT_TRUE(baton);
+    EXPECT_TRUE(context);
+    EXPECT_EQ(break_id, expected_breakpoint_id);
+    EXPECT_EQ(break_loc_id, expected_breakpoint_loc_id);
+  }
+
+protected:
+  // The debugger's initialization function can't be called with no arguments
+  // so calling it using SubsystemRAII will cause the test build to fail as
+  // SubsystemRAII will call Initialize with no arguments. As such we set it up
+  // here the usual way.
+  void SetUp() override {
+    std::call_once(TestUtilities::g_debugger_initialize_flag,
+                   []() { Debugger::Initialize(nullptr); });
+
+    // Set up the debugger, make sure that was done properly.
+    ArchSpec arch("x86_64-apple-macosx-");
+    Platform::SetHostPlatform(
+        PlatformRemoteMacOSX::CreateInstance(true, &arch));
+
+    m_debugger_sp = Debugger::CreateInstance();
+    m_debugger_sp->GetTargetList().CreateTarget(*m_debugger_sp, "", arch,
+                                                lldb_private::eLoadDependentsNo,
+                                                m_platform_sp, m_target_sp);
+    m_breakpoint_sp = m_target_sp->CreateBreakpoint(0xDEADBEEF, false, false);
+  };
+
+  static bool callback(void *baton, StoppointCallbackContext *context,
+                       lldb::user_id_t break_id, lldb::user_id_t break_loc_id) {
+    BreakpointSetCallbackTest::CheckCallbackArgs(baton, context, break_id,
+                                                 break_loc_id, 0, 0);
+    return true;
+  }
+
+  DebuggerSP m_debugger_sp;
+  PlatformSP m_platform_sp;
+  TargetSP m_target_sp;
+  BreakpointSP m_breakpoint_sp;
+  Event *m_event;
+  const ExecutionContext m_exe_ctx;
+  lldb::user_id_t expected_breakpoint_id;
+  lldb::user_id_t expected_breakpoint_loc_id;
+  SubsystemRAII<FileSystem, HostInfo, PlatformMacOSX, ProgressManager>
+      subsystems;
+};
+
+TEST_F(BreakpointSetCallbackTest, TestBreakpointSetCallback) {
+  void *baton = (void *)"hello";
+  StoppointCallbackContext context(m_event, m_exe_ctx, true);
+  m_breakpoint_sp->SetCallback(BreakpointSetCallbackTest::callback, baton,
+                               false);
+  m_breakpoint_sp->InvokeCallback(&context, 0);
+}

medismailben

This is a good start but still requires some work.

medismailben · 2024-06-18T22:40:45Z

lldb/unittests/Breakpoint/CMakeLists.txt

Do you still need this ?

Not anymore, I'll remove these.

medismailben · 2024-06-18T22:55:05Z

lldb/unittests/Callback/TestBreakpointSetCallback.cpp

+  static bool callback(void *baton, StoppointCallbackContext *context,
+                       lldb::user_id_t break_id, lldb::user_id_t break_loc_id) {
+    BreakpointSetCallbackTest::CheckCallbackArgs(baton, context, break_id,
+                                                 break_loc_id, 0, 0);


I thought the breakpoint (location) ids started at 1, how come your expected break_id & break_loc_id are 0 ?

My mistake, those should be 1 😅

medismailben · 2024-06-18T22:56:41Z

lldb/unittests/Callback/TestBreakpointSetCallback.cpp

+  StoppointCallbackContext context(m_event, m_exe_ctx, true);
+  m_breakpoint_sp->SetCallback(BreakpointSetCallbackTest::callback, baton,


You can swap these 2 lines.

medismailben · 2024-06-18T22:57:52Z

lldb/unittests/Callback/TestBreakpointSetCallback.cpp

+    EXPECT_TRUE(baton);
+    EXPECT_TRUE(context);


You should do more with these (i.e. check that actually baton point to the "hello" string).

An EXPECT_EQ for baton being "hello" right? I'll add that. Also, should I check anything more for the StoppointCallbackContext being true here?

You could check that the ExecutionContext's target matches the m_target_sp if you want.

medismailben · 2024-06-18T23:27:04Z

lldb/unittests/Callback/TestBreakpointSetCallback.cpp

+
+TEST_F(BreakpointSetCallbackTest, TestBreakpointSetCallback) {
+  void *baton = (void *)"hello";
+  StoppointCallbackContext context(m_event, m_exe_ctx, true);


It looks like m_event never gets initialized so I'd just pass a nullptr here and remove it from the test class.

medismailben · 2024-06-18T23:27:34Z

lldb/unittests/Callback/TestBreakpointSetCallback.cpp

+    EXPECT_TRUE(baton);
+    EXPECT_TRUE(context);


You could check that the ExecutionContext's target matches the m_target_sp if you want.

medismailben

LGTM, with comments addressed.

medismailben · 2024-06-20T22:07:41Z

lldb/include/lldb/lldb-private-interfaces.h

-                                      StoppointCallbackContext *context,
-                                      lldb::user_id_t break_id,
-                                      lldb::user_id_t break_loc_id);
+// typedef bool (*BreakpointHitCallback)(void *baton,


medismailben · 2024-06-20T22:08:47Z

lldb/include/lldb/lldb-private-interfaces.h

+//                                       StoppointCallbackContext *context,
+//                                       lldb::user_id_t break_id,
+//                                       lldb::user_id_t break_loc_id);
+typedef std::function<bool(void *baton, StoppointCallbackContext *context,


Can you do using instead of typedef here ?

medismailben · 2024-06-20T22:10:41Z

lldb/unittests/Callback/TestBreakpointSetCallback.cpp

+
+TEST_F(BreakpointSetCallbackTest, TestBreakpointSetCallback) {
+  void *baton = (void *)"hello";
+  TargetSP current_target = m_target_sp;


nit: you don't need to make a new various from m_target_sp, just capture it directly in the lambda.

That only works if I capture this, trying to capture just m_target_sp won't work (unless we move the target setup into the test body)

medismailben · 2024-06-21T23:07:36Z

lldb/unittests/Callback/TestBreakpointSetCallback.cpp

+using namespace lldb_private;
+using namespace lldb;
+
+#define EXPECTED_BREAKPOINT_ID 1


You can make these static constexpr lldb::user_id_t if you want

medismailben · 2024-06-21T23:08:55Z

lldb/unittests/Callback/TestBreakpointSetCallback.cpp

+TEST_F(BreakpointSetCallbackTest, TestBreakpointSetCallback) {
+  void *baton = (void *)"hello";
+  // Set up the debugger, make sure that was done properly.
+  TargetSP m_target_sp;


It's not an class member, why prepend the m_

medismailben · 2024-06-21T23:09:33Z

lldb/unittests/Callback/TestBreakpointSetCallback.cpp

+        return true;
+      },
+      baton, true);
+  ExecutionContext m_exe_ctx(m_target_sp, false);


medismailben · 2024-06-21T23:10:14Z

lldb/unittests/Callback/TestBreakpointSetCallback.cpp

+                                              m_platform_sp, m_target_sp);
+
+  // Create breakpoint
+  m_breakpoint_sp = m_target_sp->CreateBreakpoint(0xDEADBEEF, false, false);


I guess you don't need to make the breakpoint a class member here

medismailben

LGTM with comment

medismailben · 2024-06-21T23:38:35Z

lldb/unittests/Callback/TestBreakpointSetCallback.cpp

+                                lldb::user_id_t expected_breakpoint_id,
+                                lldb::user_id_t expected_breakpoint_loc_id,


no need to pass them here, since they're static

medismailben · 2024-06-22T01:19:03Z

Awesome! 🚢 it!

This commit adds a unit test for SBBreakpoint::SetCallback as it wasn't being tested before. (cherry picked from commit 347206f)

…lback-unittest Add a unit test for SBBreakpoint::SetCallback (llvm#96001)

This commit adds a unit test for SBBreakpoint::SetCallback as it wasn't being tested before.

chelcassanova requested a review from medismailben June 18, 2024 22:20

chelcassanova requested a review from JDevlieghere as a code owner June 18, 2024 22:20

llvmbot added the lldb label Jun 18, 2024

chelcassanova force-pushed the breakpoint-callback-unittest branch from dc51494 to 0dc8040 Compare June 18, 2024 22:34

medismailben requested changes Jun 18, 2024

View reviewed changes

medismailben reviewed Jun 18, 2024

View reviewed changes

chelcassanova force-pushed the breakpoint-callback-unittest branch 2 times, most recently from aa575bb to 2cfcef6 Compare June 20, 2024 21:28

medismailben approved these changes Jun 20, 2024

View reviewed changes

chelcassanova force-pushed the breakpoint-callback-unittest branch from 2cfcef6 to 86dcffc Compare June 21, 2024 22:05

medismailben approved these changes Jun 21, 2024

View reviewed changes

medismailben requested changes Jun 21, 2024

View reviewed changes

chelcassanova force-pushed the breakpoint-callback-unittest branch from 86dcffc to 44ea5e0 Compare June 21, 2024 23:18

medismailben approved these changes Jun 21, 2024

View reviewed changes

add unit test for breakpoint::setcallback

a59949c

chelcassanova force-pushed the breakpoint-callback-unittest branch from 44ea5e0 to a59949c Compare June 22, 2024 01:04

chelcassanova merged commit 347206f into llvm:main Jun 24, 2024
6 checks passed

chelcassanova deleted the breakpoint-callback-unittest branch June 24, 2024 16:51

JDevlieghere added a commit to swiftlang/llvm-project that referenced this pull request Jul 8, 2024

Merge pull request #8924 from chelcassanova/cherrypick/breakpoint-cal…

4523e78

…lback-unittest Add a unit test for SBBreakpoint::SetCallback (llvm#96001)

AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024

Add a unit test for SBBreakpoint::SetCallback (llvm#96001)

338e907

This commit adds a unit test for SBBreakpoint::SetCallback as it wasn't being tested before.

		StoppointCallbackContext context(m_event, m_exe_ctx, true);
		m_breakpoint_sp->SetCallback(BreakpointSetCallbackTest::callback, baton,

		lldb::user_id_t expected_breakpoint_id,
		lldb::user_id_t expected_breakpoint_loc_id,

Add a unit test for SBBreakpoint::SetCallback #96001

Add a unit test for SBBreakpoint::SetCallback #96001

Uh oh!

Conversation

chelcassanova commented Jun 18, 2024

Uh oh!

llvmbot commented Jun 18, 2024

Uh oh!

medismailben left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medismailben left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medismailben left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medismailben commented Jun 22, 2024

Uh oh!

Uh oh!

Uh oh!