Make SBBreakpoint/SBBreakpointLocation.SetCondition(nullptr) work again. #162370

jimingham · 2025-10-07T20:49:27Z

The addition of the StopCondition in the lldb_private layer meant that clearing a breakpoint condition with:

sb_break.SetCondition(nullptr);

now crashes. Also, GetCondition for an empty condition used to return a nullptr, but now it returns "".

This patch fixes that crash and makes the SB GetCondition always return nullptr for an empty condition.

The addition of the StopCondition in the lldb_private layer meant that clearing a breakpoint condition with: sb_break.SetCondition(nullptr); now crashes. Also, GetCondition for an empty condition used to return a nullptr, but now it returns "". This patch fixes that crash and makes the SB GetCondition always return nullptr for an empty condition.

llvmbot · 2025-10-07T20:49:59Z

@llvm/pr-subscribers-lldb

Author: None (jimingham)

Changes

The addition of the StopCondition in the lldb_private layer meant that clearing a breakpoint condition with:

sb_break.SetCondition(nullptr);

now crashes. Also, GetCondition for an empty condition used to return a nullptr, but now it returns "".

This patch fixes that crash and makes the SB GetCondition always return nullptr for an empty condition.

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

4 Files Affected:

(modified) lldb/source/API/SBBreakpoint.cpp (+9-2)
(modified) lldb/source/API/SBBreakpointLocation.cpp (+9-2)
(added) lldb/unittests/Breakpoint/BreakpointClearConditionTest.cpp (+85)
(modified) lldb/unittests/Breakpoint/CMakeLists.txt (+6-1)

diff --git a/lldb/source/API/SBBreakpoint.cpp b/lldb/source/API/SBBreakpoint.cpp
index 07c0a2ea907ba..23dba462478c9 100644
--- a/lldb/source/API/SBBreakpoint.cpp
+++ b/lldb/source/API/SBBreakpoint.cpp
@@ -275,7 +275,11 @@ void SBBreakpoint::SetCondition(const char *condition) {
   if (bkpt_sp) {
     std::lock_guard<std::recursive_mutex> guard(
         bkpt_sp->GetTarget().GetAPIMutex());
-    bkpt_sp->SetCondition(StopCondition(condition));
+    // Treat a null pointer as resetting the condition.
+    if (!condition)
+      bkpt_sp->SetCondition(StopCondition());
+    else
+      bkpt_sp->SetCondition(StopCondition(condition));
   }
 }
 
@@ -288,7 +292,10 @@ const char *SBBreakpoint::GetCondition() {
 
   std::lock_guard<std::recursive_mutex> guard(
       bkpt_sp->GetTarget().GetAPIMutex());
-  return ConstString(bkpt_sp->GetCondition().GetText()).GetCString();
+  StopCondition cond = bkpt_sp->GetCondition();
+  if (!cond)
+    return nullptr;
+  return ConstString(cond.GetText()).GetCString();
 }
 
 void SBBreakpoint::SetAutoContinue(bool auto_continue) {
diff --git a/lldb/source/API/SBBreakpointLocation.cpp b/lldb/source/API/SBBreakpointLocation.cpp
index e786435c4f8af..2feaa5c805a15 100644
--- a/lldb/source/API/SBBreakpointLocation.cpp
+++ b/lldb/source/API/SBBreakpointLocation.cpp
@@ -160,7 +160,11 @@ void SBBreakpointLocation::SetCondition(const char *condition) {
   if (loc_sp) {
     std::lock_guard<std::recursive_mutex> guard(
         loc_sp->GetTarget().GetAPIMutex());
-    loc_sp->SetCondition(StopCondition(condition));
+    // Treat a nullptr as clearing the condition
+    if (!condition)
+      loc_sp->SetCondition(StopCondition());
+    else
+      loc_sp->SetCondition(StopCondition(condition));
   }
 }
 
@@ -173,7 +177,10 @@ const char *SBBreakpointLocation::GetCondition() {
 
   std::lock_guard<std::recursive_mutex> guard(
       loc_sp->GetTarget().GetAPIMutex());
-  return ConstString(loc_sp->GetCondition().GetText()).GetCString();
+  StopCondition cond = loc_sp->GetCondition();
+  if (!cond)
+    return nullptr;
+  return ConstString(cond.GetText()).GetCString();
 }
 
 void SBBreakpointLocation::SetAutoContinue(bool auto_continue) {
diff --git a/lldb/unittests/Breakpoint/BreakpointClearConditionTest.cpp b/lldb/unittests/Breakpoint/BreakpointClearConditionTest.cpp
new file mode 100644
index 0000000000000..688c85a4a711f
--- /dev/null
+++ b/lldb/unittests/Breakpoint/BreakpointClearConditionTest.cpp
@@ -0,0 +1,85 @@
+//===-- BreakpointClearConditionTest.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/API/SBBreakpoint.h"
+#include "lldb/API/SBBreakpointLocation.h"
+#include "lldb/API/SBDebugger.h"
+#include "lldb/API/SBTarget.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 BreakpointClearConditionTest : public ::testing::Test {
+public:
+  void SetUp() override {
+    std::call_once(TestUtilities::g_debugger_initialize_flag,
+                   []() { SBDebugger::Initialize(); });
+  };
+
+  SBDebugger m_sb_debugger;
+  SubsystemRAII<FileSystem, HostInfo, PlatformMacOSX> subsystems;
+};
+
+template<typename T>
+void test_condition(T sb_object) {
+  const char *in_cond_str = "Here is a condition";
+  sb_object.SetCondition(in_cond_str);
+  // Make sure we set the condition correctly:
+  const char *out_cond_str = sb_object.GetCondition();
+  EXPECT_STREQ(in_cond_str, out_cond_str);
+  // Now unset it by passing in nullptr and make sure that works:
+  const char *empty_tokens[2] = {nullptr, ""};
+  for (auto token : empty_tokens) {
+    sb_object.SetCondition(token);
+    out_cond_str = sb_object.GetCondition();
+    // And make sure an unset condition returns nullptr:
+    EXPECT_EQ(nullptr, out_cond_str);
+  }  
+}
+
+TEST_F(BreakpointClearConditionTest, BreakpointClearConditionTest) {
+  // Set up the debugger, make sure that was done properly.
+  m_sb_debugger = SBDebugger::Create(false);
+
+  // Create target
+  SBTarget sb_target;
+  SBError error;
+  sb_target = m_sb_debugger.CreateTarget("", "x86_64-apple-macosx-", "remote-macosx",
+       /*add_dependent=*/ false, error);
+
+  EXPECT_EQ(sb_target.IsValid(), true);
+
+  // Create breakpoint
+  SBBreakpoint sb_breakpoint =
+      sb_target.BreakpointCreateByAddress(0xDEADBEEF);
+  test_condition(sb_breakpoint);
+  
+  // Address breakpoints always have one location, so we can also use this
+  // to test the location:
+  SBBreakpointLocation sb_loc = sb_breakpoint.GetLocationAtIndex(0);
+  EXPECT_EQ(sb_loc.IsValid(), true);
+  test_condition(sb_loc);
+  
+}
diff --git a/lldb/unittests/Breakpoint/CMakeLists.txt b/lldb/unittests/Breakpoint/CMakeLists.txt
index 3c234a4fea29a..005cb4787c365 100644
--- a/lldb/unittests/Breakpoint/CMakeLists.txt
+++ b/lldb/unittests/Breakpoint/CMakeLists.txt
@@ -1,10 +1,15 @@
-add_lldb_unittest(LLDBBreakpointTests
+add_lldb_unittest(LLDBBreakpointTests      
   BreakpointIDTest.cpp
+  BreakpointClearConditionTest.cpp
   WatchpointAlgorithmsTests.cpp
 
   LINK_COMPONENTS
     Support
   LINK_LIBS
+    liblldb
     lldbBreakpoint
     lldbCore
+    LLVMTestingSupport
+    lldbUtilityHelpers
+    lldbPluginPlatformMacOSX
   )

github-actions · 2025-10-07T20:51:38Z

✅ With the latest revision this PR passed the C/C++ code formatter.

JDevlieghere · 2025-10-07T20:53:11Z

lldb/unittests/Breakpoint/BreakpointClearConditionTest.cpp

+//===-- BreakpointClearConditionTest.cpp
+//--------------------------------------------===//


Suggested change

//===-- BreakpointClearConditionTest.cpp

//--------------------------------------------===//

//===----------------------------------------------------------------------===//

JDevlieghere · 2025-10-07T20:54:29Z

lldb/unittests/API/SBBreakpointClearConditionTest.cpp

+TEST_F(BreakpointClearConditionTest, BreakpointClearConditionTest) {
+  // Set up the debugger, make sure that was done properly.
+  m_sb_debugger = SBDebugger::Create(false);
+
+  // Create target
+  SBTarget sb_target;
+  SBError error;
+  sb_target = m_sb_debugger.CreateTarget("", "x86_64-apple-macosx-", "remote-macosx",
+       /*add_dependent=*/ false, error);
+
+  EXPECT_EQ(sb_target.IsValid(), true);
+
+  // Create breakpoint
+  SBBreakpoint sb_breakpoint =
+      sb_target.BreakpointCreateByAddress(0xDEADBEEF);
+  test_condition(sb_breakpoint);
+
+  // Address breakpoints always have one location, so we can also use this
+  // to test the location:
+  SBBreakpointLocation sb_loc = sb_breakpoint.GetLocationAtIndex(0);
+  EXPECT_EQ(sb_loc.IsValid(), true);
+  test_condition(sb_loc);
+
+}


You will have to move this test in the unittests/API/ directory. You cannot have a unit test that links both the public and the private API.

JDevlieghere · 2025-10-07T20:55:17Z

lldb/unittests/Breakpoint/CMakeLists.txt

  LINK_COMPONENTS
    Support
  LINK_LIBS
+    liblldb


Only the API unit tests are allowed to link liblldb.

This is not enforced, as in the test builds and runs successfully where it is...

I have no problem moving it, but if you were expecting this to have been a real as opposed to a theoretical thing, it isn't...

Also, if this is a rule, it would be good to have it documented somewhere.

When you mix static and dynamic libraries that contain the same code, you end up with two copies of the same symbols which leads to various hard-to-debug issues (like having two copies of globals). When you say it "works" you mean you don't notice any of these issues for this particular test, but that doesn't make it any less wrong :-)

I like the idea of enforcing this at the CMake layer. I've filed: #162378

jimingham · 2025-10-07T23:58:37Z

Grrr... This is failing the Linux test bot because someone had previously changed an API in Stream.h but didn't update the documentation and for some reason the test I added included that in a way that caused a -Wdocumentation warning, which caused the Linux bot - which runs -Werror - to fail.
I fixed that in:

#162401

lldb/unittests/API/SBBreakpointClearConditionTest.cpp

…aders. This works around the -Wdocumentation error when building the test.

Use the LLDB.h so the test can test documentation as a side-effect. Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>

Correct the dash pattern. Co-authored-by: Jonas Devlieghere <jonas@devlieghere.com>

in the GitHub UI.

…in. (llvm#162370) The addition of the StopCondition in the lldb_private layer meant that clearing a breakpoint condition with: sb_break.SetCondition(nullptr); now crashes. Also, GetCondition for an empty condition used to return a nullptr, but now it returns "". This patch fixes that crash and makes the SB GetCondition always return nullptr for an empty condition. (cherry picked from commit f3e2c20)

llvm-ci · 2025-10-08T20:03:55Z

LLVM Buildbot has detected a new failure on builder cross-project-tests-sie-ubuntu-dwarf5 running on doug-worker-1b while building lldb at step 4 "cmake-configure".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/163/builds/27856