[lldb] Disable variable watchpoints when going out of scope

[mib]

Summary

If we use a variable watchpoint with a condition using a scope variable,
if we go out-of-scope, the watpoint remains active which can the
expression evaluator to fail to parse the watchpoint condition (because
of the missing varible bindings).

This was discovered after watchpoint_callback.test started failing on
the green dragon bot.

This patch should address that issue by setting an internal breakpoint
on the return addresss of the current frame when creating a variable
watchpoint. The breakpoint has a callback that will disable the watchpoint
if the the breakpoint execution context matches the watchpoint execution
context.

This is only enabled for local variables.

This patch also re-enables the failing test following e1086384e584.

rdar://109574319

Signed-off-by: Med Ismail Bennani <[e-mail]>

[bulbazord]

lldb/source/Breakpoint/Watchpoint.cpp

86

Should you also verify that the frame_sp you got isn't nullptr? If it'll never be null, consider passing a StackFrame & instead.

103–104

Is it even possible to have an ExecutionContext created from a StackFrame that doesn't have a valid Target? I wonder if we should assert that we got one here if not.

129–133

nit: You can merge this into one:

if (!baton || !context)
  return false;

140–141

Any reason to not use LLDB_LOG here? You'll get __FILE__ and __FUNCTION__ included.

152

This should be return false;

165

This should be return true; no? You could invert the condition to be consistent with the patterns above of "return false if some condition wasn't met".

[mib]

lldb/source/Breakpoint/Watchpoint.cpp

103–104

There is no ExecutionContext::IsValid method, so if you created an ExecutionContext with a bogus frame, you might get a nullptr.

165

We want to always return false since this is a breakpoint callback, we never want to stop.

[mib]

lldb/source/Breakpoint/Watchpoint.cpp

140–141

I just wanted to stay consistent with the rest of the file. May we be should standardize the logging format.

[aprantl]

Just so I understand the limitations:
This works for

int main() {
  int val = 0;
  // Break here
  val++;
  val++;
  return 0;
}

but not for

int main() {
  {
    int val = 0;
    // Break here
    val++;
    val++;
  }
  {
    int other = 42;
   printf("assuming that other and val aliases");
  }
  return 0;
}

?

To be clear, I think this is still useful!

lldb/include/lldb/Breakpoint/Watchpoint.h

93

Doxygen comment?

109

s/which/to/

lldb/source/Breakpoint/Watchpoint.cpp

86

I subjectively feel like I've received a crash log for every single unchecked dereference in the LLDB code so far ;-)

lldb/source/Commands/CommandObjectWatchpoint.cpp

956

Convert to early exit?

[mib]

Address @bulbazord & @aprantl comments.

[mib]

In D151366#4370123, @aprantl wrote:

Just so I understand the limitations:
This works for

int main() {
  int val = 0;
  // Break here
  val++;
  val++;
  return 0;
}

but not for

int main() {
  {
    int val = 0;
    // Break here
    val++;
    val++;
  }
  {
    int other = 42;
    printf("assuming that other and val aliases");
  }
  return 0;
}

?

To be clear, I think this is still useful!

@aprantl Do you know if we can detect the end of the scope ? I'm not sure it's possible currently ... But even if we could do it, that wouldn't cover the following case:

int main() {
  int i = 0;
  {
back_in_scope:
    int val = 0;
    i++;
    // Break here
    val++;
    val++;
  }
  {
    if (i == 1)
      goto back_in_scope;
    printf("assuming that other and val aliases");
  }
  return 0;
}

If we disable the val variable watchpoint at the end of the scope, if we get back into it (because of a goto or whatever), the watchpoint won't trigger because it will disabled (by leaving the scope).

I think in order to solve these kind of problem we should do proper static analysis to be able to answer those questions.

[bulbazord]

I have no objections but you should probably wait for others to take another look.

[aprantl]

@aprantl Do you know if we can detect the end of the scope ? I'm not sure it's possible currently ... But even if we could do it, that wouldn't cover the following case:

When setting a variable watchpoint, you would have to store the scope the variable is in, and then ignore all hits that are (1) on the same thread but (2) where the scope is not any of the lexical scopes in any of the stack frames. Because you still want to find modifications in child frames or on other threads.

[mib]

In D151366#4372731, @aprantl wrote:

@aprantl Do you know if we can detect the end of the scope ? I'm not sure it's possible currently ... But even if we could do it, that wouldn't cover the following case:

When setting a variable watchpoint, you would have to store the scope the variable is in, and then ignore all hits that are (1) on the same thread but (2) where the scope is not any of the lexical scopes in any of the stack frames. Because you still want to find modifications in child frames or on other threads.

As discussed offline, watchpoints still have many shortcomings that can't be addressed all at once, so I'll land this patch and document the various issues discussed here so hopefully we can find a solution for those.

[JDevlieghere]

lldb/source/Breakpoint/Watchpoint.cpp

127

I think this should return an llvm::Error and log it at the call site. For every early return we know what went wrong. Same for SetupVariableWatchpointDisabler.

140

Doesn't FUNCTION already include Watchpoint::? Also I think there's a flag to include the function name in the log, so this is redundant.

lldb/source/Commands/CommandObjectWatchpoint.cpp

959

You wouldn't need the (uint64_t) cast if you used formatv. This also should use a C++ style cast (https://discourse.llvm.org/t/rfc-add-preferred-casting-style-to-coding-standards/70793/4).

960–975

Status::AsCString isn't trivial so probably worth calling this one and storing it in a temporary variable.

[mib]

lldb/source/Breakpoint/Watchpoint.cpp

127

This is a WatchpointCallback so it have to match a specific function signature (and return a bool). I could change SetupVariableWatchpointDisabler to return an llvm::Error in a follow-up

140

No, it only prints the method name, LLVM_PRETTY_FUNCTION prints everything but I think that macro is not defined with other compiler.

[mib]

lldb/source/Commands/CommandObjectWatchpoint.cpp

959

Good point, but this code was already here before my patch, I just moved around to have early returns.

[JDevlieghere]

lldb/source/Breakpoint/Watchpoint.cpp

127

That's unfortunate but makes sense. Thanks for clarifying.

140

LLVM_PRETTY_FUNCTION is an llvm define, it expands to the appropriate macro based on the host compiler. You can totally use it but you must include Compiler.h.

[antmo]

Hi @mib,
I think this broke lldb-arm-ubuntu bot (LocalVariableWatchpointDisabler.test failure): https://lab.llvm.org/buildbot/#/builders/17/builds/38136
Also visible on lldb-aarch64-windows here (TestVarPath.py issue is something else): https://lab.llvm.org/buildbot/#/builders/219/builds/3048
Could you please take a look ?

[jasonmolenda]

In D151366#4375959, @antmo wrote:

Hi @mib,
I think this broke lldb-arm-ubuntu bot (LocalVariableWatchpointDisabler.test failure): https://lab.llvm.org/buildbot/#/builders/17/builds/38136
Also visible on lldb-aarch64-windows here (TestVarPath.py issue is something else): https://lab.llvm.org/buildbot/#/builders/219/builds/3048
Could you please take a look ?

Thanks for pointing out the bot failures, I hadn't noticed them yet. I've reverted Ismail's change until he can look into it.

lldb/include/lldb/Breakpoint/Watchpoint.h

public:
   std::string GetWatchSpec();
   void SetWatchSpec(const std::string &str);
 
   // Snapshot management interface.
   bool IsWatchVariable() const;
   void SetWatchVariable(bool val);
   bool CaptureWatchedValue(const ExecutionContext &exe_ctx);
 
+  /// \struct WatchpointVariableContext
+  /// \brief Represents the context of a watchpoint variable.
+  ///
+  /// This struct encapsulates the information related to a watchpoint variable,
+  /// including the watch ID and the execution context in which it is being
+  /// used. This struct is passed as a Baton to the \b
+  /// VariableWatchpointDisabler breakpoint callback.
+  struct WatchpointVariableContext {
+    /// \brief Constructor for WatchpointVariableContext.
+    /// \param watch_id The ID of the watchpoint.
+    /// \param exe_ctx The execution context associated with the watchpoint.
+    WatchpointVariableContext(lldb::watch_id_t watch_id,
+                              ExecutionContext exe_ctx)
+        : watch_id(watch_id), exe_ctx(exe_ctx) {}
+
+    lldb::watch_id_t watch_id; ///< The ID of the watchpoint.
+    ExecutionContext
+        exe_ctx; ///< The execution context associated with the watchpoint.
+  };
+
+  class WatchpointVariableBaton : public TypedBaton<WatchpointVariableContext> {
+  public:
+    WatchpointVariableBaton(std::unique_ptr<WatchpointVariableContext> Data)
+        : TypedBaton(std::move(Data)) {}
+  };
+
+  bool SetupVariableWatchpointDisabler(lldb::StackFrameSP frame_sp) const;
+
+  /// Callback routine to disable the watchpoint set on a local variable when
+  ///  it goes out of scope.
+  static bool VariableWatchpointDisabler(
+      void *baton, lldb_private::StoppointCallbackContext *context,
+      lldb::user_id_t break_id, lldb::user_id_t break_loc_id);
+
   void GetDescription(Stream *s, lldb::DescriptionLevel level);
   void Dump(Stream *s) const override;
   void DumpSnapshots(Stream *s, const char *prefix = nullptr) const;
   void DumpWithLevel(Stream *s, lldb::DescriptionLevel description_level) const;
   Target &GetTarget() { return m_target; }
   const Status &GetError() { return m_error; }
 
   /// Returns the WatchpointOptions structure set for this watchpoint.

lldb/source/Breakpoint/Watchpoint.cpp

 // contained in a "Baton" subclass.
 void Watchpoint::SetCallback(WatchpointHitCallback callback,
                              const BatonSP &callback_baton_sp,
                              bool is_synchronous) {
   m_options.SetCallback(callback, callback_baton_sp, is_synchronous);
   SendWatchpointChangedEvent(eWatchpointEventTypeCommandChanged);
 }
 
+bool Watchpoint::SetupVariableWatchpointDisabler(StackFrameSP frame_sp) const {
+  if (!frame_sp)
+    return false;
+
+  ThreadSP thread_sp = frame_sp->GetThread();
+  if (!thread_sp)
+    return false;
+
+  uint32_t return_frame_index =
+      thread_sp->GetSelectedFrameIndex(DoNoSelectMostRelevantFrame) + 1;
+  if (return_frame_index >= LLDB_INVALID_FRAME_ID)
+    return false;
+
+  StackFrameSP return_frame_sp(
+      thread_sp->GetStackFrameAtIndex(return_frame_index));
+  if (!return_frame_sp)
+    return false;
+
+  ExecutionContext exe_ctx(return_frame_sp);
+  TargetSP target_sp = exe_ctx.GetTargetSP();
+  if (!target_sp)
+    return false;
+
+  Address return_address(return_frame_sp->GetFrameCodeAddress());
+  lldb::addr_t return_addr = return_address.GetLoadAddress(target_sp.get());
+  if (return_addr == LLDB_INVALID_ADDRESS)
+    return false;
+
+  BreakpointSP bp_sp = target_sp->CreateBreakpoint(
+      return_addr, /*internal=*/true, /*request_hardware=*/false);
+  if (!bp_sp || !bp_sp->HasResolvedLocations())
+    return false;
+
+  auto wvc_up = std::make_unique<WatchpointVariableContext>(GetID(), exe_ctx);
+  auto baton_sp = std::make_shared<WatchpointVariableBaton>(std::move(wvc_up));
+  bp_sp->SetCallback(VariableWatchpointDisabler, baton_sp);
+  bp_sp->SetOneShot(true);
+  bp_sp->SetBreakpointKind("variable watchpoint disabler");
+  return true;
+}
+
+bool Watchpoint::VariableWatchpointDisabler(void *baton,
+                                            StoppointCallbackContext *context,
+                                            user_id_t break_id,
+                                            user_id_t break_loc_id) {
+  assert(baton && "null baton");
+  if (!baton || !context)
+    return false;
+
+  Log *log = GetLog(LLDBLog::Watchpoints);
+
+  WatchpointVariableContext *wvc =
+      static_cast<WatchpointVariableContext *>(baton);
+
+  LLDB_LOGF(log, "Watchpoint::%s called by breakpoint %" PRIu64 ".%" PRIu64,
+            __FUNCTION__, break_id, break_loc_id);
+
+  if (wvc->watch_id == LLDB_INVALID_WATCH_ID)
+    return false;
+
+  TargetSP target_sp = context->exe_ctx_ref.GetTargetSP();
+  if (!target_sp)
+    return false;
+
+  ProcessSP process_sp = target_sp->GetProcessSP();
+  if (!process_sp)
+    return false;
+
+  WatchpointSP watch_sp =
+      target_sp->GetWatchpointList().FindByID(wvc->watch_id);
+  if (!watch_sp)
+    return false;
+
+  if (wvc->exe_ctx == context->exe_ctx_ref) {
+    LLDB_LOGF(log,
+              "Watchpoint::%s callback for watchpoint %" PRId32
+              " matched internal breakpoint execution context",
+              __FUNCTION__, watch_sp->GetID());
+    process_sp->DisableWatchpoint(watch_sp.get());
+    return false;
+  }
+  LLDB_LOGF(log,
+            "Watchpoint::%s callback for watchpoint %" PRId32
+            " didn't match internal breakpoint execution context",
+            __FUNCTION__, watch_sp->GetID());
+  return false;
+}
+
 void Watchpoint::ClearCallback() {
   m_options.ClearCallback();
   SendWatchpointChangedEvent(eWatchpointEventTypeCommandChanged);
 }
 
 void Watchpoint::SetDeclInfo(const std::string &str) { m_decl_str = str; }
 
 std::string Watchpoint::GetWatchSpec() { return m_watch_spec_str; }

lldb/source/Commands/CommandObjectWatchpoint.cpp

 //===-- CommandObjectWatchpoint.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 "CommandObjectWatchpoint.h"
 #include "CommandObjectWatchpointCommand.h"
 
+#include <memory>
 #include <vector>
 
 #include "llvm/ADT/StringRef.h"
 
 #include "lldb/Breakpoint/Watchpoint.h"
 #include "lldb/Breakpoint/WatchpointList.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Host/OptionParser.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandOptionArgumentTable.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
+#include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
 #include "lldb/Target/StackFrame.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Utility/StreamString.h"
 
 using namespace lldb;
 using namespace lldb_private;
bool DoExecute(Args &command, CommandReturnObject &result) override {
     }
 
     // Now it's time to create the watchpoint.
     uint32_t watch_type = m_option_watchpoint.watch_type;
 
     error.Clear();
     WatchpointSP watch_sp =
         target->CreateWatchpoint(addr, size, &compiler_type, watch_type, error);
-    if (watch_sp) {
-      watch_sp->SetWatchSpec(command.GetArgumentAtIndex(0));
-      watch_sp->SetWatchVariable(true);
-      if (var_sp && var_sp->GetDeclaration().GetFile()) {
+    if (!watch_sp) {
+      result.AppendErrorWithFormat(
+          "Watchpoint creation failed (addr=0x%" PRIx64 ", size=%" PRIu64
+          ", variable expression='%s').\n",
+          addr, (uint64_t)size, command.GetArgumentAtIndex(0));
+      if (error.AsCString(nullptr))
+        result.AppendError(error.AsCString());
+      return result.Succeeded();
+    }
+
+    if (var_sp) {
+      if (var_sp->GetDeclaration().GetFile()) {
         StreamString ss;
         // True to show fullpath for declaration file.
         var_sp->GetDeclaration().DumpStopContext(&ss, true);
         watch_sp->SetDeclInfo(std::string(ss.GetString()));
       }
-      output_stream.Printf("Watchpoint created: ");
-      watch_sp->GetDescription(&output_stream, lldb::eDescriptionLevelFull);
-      output_stream.EOL();
-      result.SetStatus(eReturnStatusSuccessFinishResult);
-    } else {
-      result.AppendErrorWithFormat(
-          "Watchpoint creation failed (addr=0x%" PRIx64 ", size=%" PRIu64
-          ", variable expression='%s').\n",
-          addr, (uint64_t)size, command.GetArgumentAtIndex(0));
-      if (error.AsCString(nullptr))
-        result.AppendError(error.AsCString());
-    }
+      if (var_sp->GetScope() == eValueTypeVariableLocal)
+        watch_sp->SetupVariableWatchpointDisabler(m_exe_ctx.GetFrameSP());
+    }
 
     return result.Succeeded();
   }
 
 private:
   OptionGroupOptions m_option_group;
   OptionGroupWatchpoint m_option_watchpoint;
 };

lldb/test/Shell/ScriptInterpreter/Lua/watchpoint_callback.test

-# XFAIL: system-netbsd, system-darwin
+# XFAIL: system-netbsd
 # RUN: %clang_host -x c %S/Inputs/val.c -g -o %t
 # RUN: %lldb -b -s %S/Inputs/watchpoint1.in --script-language lua %t 2>&1 | FileCheck %S/Inputs/watchpoint1.in
 # RUN: %lldb -b -s %S/Inputs/watchpoint2.in --script-language lua %t 2>&1 | FileCheck %S/Inputs/watchpoint2.in
 # RUN: %lldb -b -s %S/Inputs/watchpoint3.in --script-language lua %t 2>&1 | FileCheck %S/Inputs/watchpoint3.in

lldb/test/Shell/Watchpoint/Inputs/val.c

+int main() {
+  int val = 0;
+  // Break here
+  val++;
+  val++;
+  return 0;
+}

lldb/test/Shell/Watchpoint/Inputs/watchpoint.in

+breakpoint set -p "Break here"
+r
+watchpoint set variable val
+watchpoint modify -c "val == 1"
+c
+# CHECK: Watchpoint 1 hit:
+# CHECK-NEXT: old value: 0
+# CHECK-NEXT: new value: 1
+# CHECK-NEXT: Process {{[0-9]+}} resuming
+# CHECK-NEXT: Process {{[0-9]+}} stopped
+# CHECK-NEXT: {{.*}} stop reason = watchpoint 1
+c
+# CHECK: Process {{[0-9]+}} exited with status = 0 (0x00000000)

lldb/test/Shell/Watchpoint/LocalVariableWatchpointDisabler.test

+# XFAIL: system-netbsd
+# RUN: %clang_host -x c %S/Inputs/val.c -g -o %t
+# RUN: %lldb -b -s %S/Inputs/watchpoint.in %t 2>&1 | FileCheck %S/Inputs/watchpoint.in