Make the sanitizer Notify callbacks asynchronous

[jingham]

Summary

lldb handles breakpoints in two phases, the "sync" and "async" phase. The synchronous phase happens early in the event handling, and is for when you need to do some work before anyone else gets a chance to look at the breakpoint. It's used by the dynamic loader plugins, for instance.

As such, we're in a place where we haven't quite figured out about the current event, and are not in a state to handle another one. That makes running the expression evaluator in synchronous callbacks flakey. The Sanitizer callbacks were all registered as synchronous, even though their primary job is to call report generation functions in the target. TTTT, calling functions works better than I would have expected, but if you rapidly continue enough times over a sequence of sanitizer reports, you eventually confuse the event state machine.

I bet with enough thinking we could make expression evaluation in sync callbacks work, but it would further complicate an already complex system. And there's no reason for the sanitizer callbacks to be synchronous, they are very like user report functions in a breakpoint, and should run in the same way. So the simpler path of converting these to async seems the better one.

This patch switches the report functions from sync to async, adds some testing for continuing past sequential UBSan reports, and adds a comment telling people not to use expressions in sync callbacks. The test is not great, in that I can get this same test code to misbehave quite regularly in Xcode, but I couldn't reproduce enough of the Xcode environment in a test to produce a reliably failing test, so this test for the most part passes even without the fix.

[JDevlieghere]

LGTM

lldb/source/Plugins/InstrumentationRuntime/ASan/InstrumentationRuntimeASan.cpp

301–310

Might be nice spell this out explicitly, like we do for the internal and hardware arguments to CreateBreakpoint

[jingham]

I had to fix one other thing. I had arbitrarily made the decision that if an internal breakpoint was hit while running an expression we should stop before running its callbacks. At the time we only had internal breakpoints with sync callbacks (again mostly dynamic loader plugins) so there wasn't really an example to reason on. But with these sanitizer ones now async, we can hit this issue. The problem with hitting a breakpoint while running an expression is that those breakpoint commands could run the target in arbitrary ways, which could hit this breakpoint, and then call other commands, etc, and right now the Command Interpreter can only be invoked recursively with some care, so I outlaw that. But I think it's better for internal breakpoints to let their callbacks run, and leave it up to the callback how it wants to do its business. In the case of the sanitizers, they just don't run their reports if there's a sanitizer issue while calling a function, in other cases we're just reading memory, etc.

So I added that change to StopInfo, and updated the patch. I'll let it sit for a bit in case anyone wants to have a look. I also made variables for the "false" I was passing to SetCallback as Jonas suggested.

[jingham]

Handled hitting the sanitizer breakpoint while calling a function in StopInfoBreakpoint.

lldb/include/lldb/Breakpoint/Breakpoint.h

public:
   ///
   /// \param[in] callback
   ///    The method that will get called when the breakpoint is hit.
   /// \param[in] baton
   ///    A void * pointer that will get passed back to the callback function.
   /// \param[in] is_synchronous
   ///    If \b true the callback will be run on the private event thread
   ///    before the stop event gets reported.  If false, the callback will get
-  ///    handled on the public event thread after the stop has been posted.
+  ///    handled on the public event thread while the stop event is being
+  ///    pulled off the event queue.
+  ///    Note: synchronous callbacks cannot cause the target to run, in
+  ///    particular, they should not try to run the expression evaluator.
   void SetCallback(BreakpointHitCallback callback, void *baton,
                    bool is_synchronous = false);
 
   void SetCallback(BreakpointHitCallback callback,
                    const lldb::BatonSP &callback_baton_sp,
                    bool is_synchronous = false);
 
   void ClearCallback();

lldb/source/Plugins/InstrumentationRuntime/ASan/InstrumentationRuntimeASan.cpp

void InstrumentationRuntimeASan::Activate() {
   if (!symbol->ValueIsAddress() || !symbol->GetAddressRef().IsValid())
     return;
 
   Target &target = process_sp->GetTarget();
   addr_t symbol_address = symbol->GetAddressRef().GetOpcodeLoadAddress(&target);
 
   if (symbol_address == LLDB_INVALID_ADDRESS)
     return;
 
-  bool internal = true;
-  bool hardware = false;
+  const bool internal = true;
+  const bool hardware = false;
+  const bool sync = false;
   Breakpoint *breakpoint =
       process_sp->GetTarget()
           .CreateBreakpoint(symbol_address, internal, hardware)
           .get();
   breakpoint->SetCallback(InstrumentationRuntimeASan::NotifyBreakpointHit, this,
-                          true);
+                          sync);
   breakpoint->SetBreakpointKind("address-sanitizer-report");
   SetBreakpointID(breakpoint->GetID());
 
   SetActive(true);
 }
 
 void InstrumentationRuntimeASan::Deactivate() {
   if (GetBreakpointID() != LLDB_INVALID_BREAK_ID) {
     ProcessSP process_sp = GetProcessSP();
     if (process_sp) {
       process_sp->GetTarget().RemoveBreakpointByID(GetBreakpointID());
       SetBreakpointID(LLDB_INVALID_BREAK_ID);
     }
   }
   SetActive(false);
 }

lldb/source/Plugins/InstrumentationRuntime/MainThreadChecker/InstrumentationRuntimeMainThreadChecker.cpp

void InstrumentationRuntimeMainThreadChecker::Activate() {
   if (symbol_address == LLDB_INVALID_ADDRESS)
     return;
 
   Breakpoint *breakpoint =
       process_sp->GetTarget()
           .CreateBreakpoint(symbol_address, /*internal=*/true,
                             /*hardware=*/false)
           .get();
+  const bool sync = false;
   breakpoint->SetCallback(
-      InstrumentationRuntimeMainThreadChecker::NotifyBreakpointHit, this, true);
+      InstrumentationRuntimeMainThreadChecker::NotifyBreakpointHit, this, sync);
   breakpoint->SetBreakpointKind("main-thread-checker-report");
   SetBreakpointID(breakpoint->GetID());
 
   SetActive(true);
 }
 
 void InstrumentationRuntimeMainThreadChecker::Deactivate() {
   SetActive(false);

lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

if (!symbol->ValueIsAddress() || !symbol->GetAddressRef().IsValid())
     return;
 
   Target &target = process_sp->GetTarget();
   addr_t symbol_address = symbol->GetAddressRef().GetOpcodeLoadAddress(&target);
 
   if (symbol_address == LLDB_INVALID_ADDRESS)
     return;
 
-  bool internal = true;
-  bool hardware = false;
+  const bool internal = true;
+  const bool hardware = false;
+  const bool sync = false;
   Breakpoint *breakpoint =
       process_sp->GetTarget()
           .CreateBreakpoint(symbol_address, internal, hardware)
           .get();
   breakpoint->SetCallback(InstrumentationRuntimeTSan::NotifyBreakpointHit, this,
-                          true);
+                          sync);
   breakpoint->SetBreakpointKind("thread-sanitizer-report");
   SetBreakpointID(breakpoint->GetID());
 
   SetActive(true);
 }
 
 void InstrumentationRuntimeTSan::Deactivate() {
   if (GetBreakpointID() != LLDB_INVALID_BREAK_ID) {

lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp

void InstrumentationRuntimeUBSan::Activate() {
   if (symbol_address == LLDB_INVALID_ADDRESS)
     return;
 
   Breakpoint *breakpoint =
       process_sp->GetTarget()
           .CreateBreakpoint(symbol_address, /*internal=*/true,
                             /*hardware=*/false)
           .get();
+  const bool sync = false;
   breakpoint->SetCallback(InstrumentationRuntimeUBSan::NotifyBreakpointHit,
-                          this, true);
+                          this, sync);
   breakpoint->SetBreakpointKind("undefined-behavior-sanitizer-report");
   SetBreakpointID(breakpoint->GetID());
 
   SetActive(true);
 }
 
 void InstrumentationRuntimeUBSan::Deactivate() {
   SetActive(false);

lldb/source/Target/StopInfo.cpp

if (thread_sp) {
               return;
             }
 
             LLDB_LOGF(log, "StopInfoBreakpoint::PerformAction - Hit a "
                            "breakpoint while running an expression,"
                            " not running commands to avoid recursion.");
             bool ignoring_breakpoints =
                 process->GetIgnoreBreakpointsInExpressions();
-            if (ignoring_breakpoints) {
-              m_should_stop = false;
-              // Internal breakpoints will always stop.
-              for (size_t j = 0; j < num_owners; j++) {
-                lldb::BreakpointLocationSP bp_loc_sp =
-                    bp_site_sp->GetOwnerAtIndex(j);
-                if (bp_loc_sp->GetBreakpoint().IsInternal()) {
-                  m_should_stop = true;
-                  break;
-                }
-              }
-            } else {
-              m_should_stop = true;
-            }
+            // Internal breakpoints should be allowed to do their job, we
+            // can make sure they don't do anything that would cause recursive
+            // command execution:
+            if (!m_was_all_internal) {
+              m_should_stop = !ignoring_breakpoints;
               LLDB_LOGF(log,
                         "StopInfoBreakpoint::PerformAction - in expression, "
                         "continuing: %s.",
                         m_should_stop ? "true" : "false");
               Debugger::ReportWarning(
-                "hit breakpoint while running function, skipping commands and "
-                "conditions to prevent recursion",
+                  "hit breakpoint while running function, skipping commands "
+                  "and conditions to prevent recursion",
                     process->GetTarget().GetDebugger().GetID());
               return;
             }
+          }
 
           StoppointCallbackContext context(event_ptr, exe_ctx, false);
 
           // For safety's sake let's also grab an extra reference to the
           // breakpoint owners of the locations we're going to examine, since
           // the locations are going to have to get back to their breakpoints,
           // and the locations don't keep their owners alive.  I'm just
           // sticking the BreakpointSP's in a vector since I'm only using it to

lldb/test/API/functionalities/ubsan/basic/TestUbsanBasic.py

def ubsan_tests(self):
         json_line = '\n'.join(output_lines[2:])
         data = json.loads(json_line)
 
         self.assertEqual(data["instrumentation_class"], "UndefinedBehaviorSanitizer")
         self.assertEqual(data["description"], "misaligned-pointer-use")
         self.assertEqual(os.path.basename(data["filename"]), "main.c")
         self.assertEqual(data["line"], self.line_align)
 
-        self.runCmd("continue")
+        for count in range(0,8):
+            process.Continue()
+            stop_reason = thread.GetStopReason()
+            self.assertEqual(stop_reason, lldb.eStopReasonInstrumentation,
+                             "Round {0} wasn't instrumentation".format(count))
+

lldb/test/API/functionalities/ubsan/basic/main.c

 int main() {
   int data[4];
-  return *(int *)(((char *)&data[0]) + 2); // align line
+  int result = *(int *)(((char *)&data[0]) + 2); // align line
+
+  int *p = data + 5;  // Index 5 out of bounds for type 'int [4]'
+  *p = data + 5;
+  *p = data + 5;
+  *p = data + 5;
+  *p = data + 5;
+  *p = data + 5;
+  *p = data + 5;
+  *p = data + 5;
+  *p = data + 5;
+
+  return 0;
 }