Handle hardcoded breakpoints on Windows (e.g., int3 or __debugbreak())

[amccarth]

Summary

No hurry on the review. This can go in next year. :-)

This required a little rejiggering of ProcessWindowsLive::RefreshStateAfterStop in order to treat an unexpected breakpoint opcode as an exception worth stopping on.

Added a Windows-only test to check it out. In the future, the test probably could be generalized to handle other platforms.

[zturner]

source/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp

601–605

When you do a disassembly, does the arrow point to the int 3 or to the instruction after the int 3?

Also what does the output look like when you do a backtrace?

[labath]

Linux tests the same functionality in TestBuiltinTrap.py. Would it make sense to merge these two tests, given that the only difference is which compiler intrinsic is used to generate the int3 trap?

[amccarth]

In D15834#318623, @labath wrote:

Linux tests the same functionality in TestBuiltinTrap.py. Would it make sense to merge these two tests, given that the only difference is which compiler intrinsic is used to generate the int3 trap?

Yes, it makes sense to combine the tests. The existing test does use the SB API very well, and TestBuiltinTrap.py is in a Linux-specific part of the test tree, so this may be more of re-working the Linux test to work much like this new one.

source/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp

601–605

It points to the instruction after the int 3.

(lldb) thread backtrace

• thread #1: tid = 0x4d70, 0x00a8903b a.out`main(argc=1, argv=0x0019c800) + 59 at main.c:12, stop reason = Exception 0x80000003 encountered at address 0xa8903a
  • frame #0: 0x00a8903b a.out`main(argc=1, argv=0x0019c800) + 59 at main.c:12 frame #1: 0x00a89157 a.out`.text$mn + 254

[jingham]

Shouldn't we just remove the Linux test case in favor of your test case? The notion of a builtin_trap of some sort or other seems general. And, it looks like both the Linux port and the Windows port needed to do something to handle this specially so having a generic test might point out that this needs work the next time there is a new OS port. So having another test on the Linux side just seems confusing.

[labath]

I agree that the new test is better than the existing linux one, which can therefore be removed. If you wish, I can do it after you get this in.

[amccarth]

I've rolled bits of TestBuiltinTrap.py into the new test and deleted the old one.

This still works on Windows. I'm not set up to test it on Linux.

[amccarth]

PTAL: I've enhanced the test to do a couple extra checks that TestBuiltinTrap.py was doing, and I've eliminated that old test.

I believe it should work on Linux as well as Windows, but I'm not set up with a Linux machine right now. If someone could try this patch on Linux, I'd appreciate it.

If/when this lands, I'll update https://llvm.org/bugs/show_bug.cgi?id=15936 to refer to the new test.

[labath]

Hm.. it seems I have misunderstood the purpose of __builtin_trap and the respective test. The function seems to generate in illegal opcode (ud2) instead of the expected int3 breakpoint. And according to the original commit message, it seems the test was added to verify that this illegal instruction does not confuse lldb/llvm. After ud2/SIGILL, it is not possible (without additional tricks) to continue the app like your test does.

So it seems that these are two tests are not that related after all, and we might as well keep them separate. I am sorry for the extra work caused by this.

[zturner]

In D15834#319237, @amccarth wrote:

In D15834#318623, @labath wrote:

Linux tests the same functionality in TestBuiltinTrap.py. Would it make sense to merge these two tests, given that the only difference is which compiler intrinsic is used to generate the int3 trap?

Yes, it makes sense to combine the tests. The existing test does use the SB API very well, and TestBuiltinTrap.py is in a Linux-specific part of the test tree, so this may be more of re-working the Linux test to work much like this new one.

Do clang / gcc have an intrinsic to generate an int3?

[amccarth]

In D15834#321261, @zturner wrote:

In D15834#319237, @amccarth wrote:

In D15834#318623, @labath wrote:

Linux tests the same functionality in TestBuiltinTrap.py. Would it make sense to merge these two tests, given that the only difference is which compiler intrinsic is used to generate the int3 trap?

Yes, it makes sense to combine the tests. The existing test does use the SB API very well, and TestBuiltinTrap.py is in a Linux-specific part of the test tree, so this may be more of re-working the Linux test to work much like this new one.

Do clang / gcc have an intrinsic to generate an int3?

If not, it would be easy to restore TestBuiltinTrap.py and change my test to use asm ("int 3"); on Linux.

[labath]

I looked for a while this morning, but I couldn't find any. You can do it with inline asm, but that's about it, I think.

[jingham]

On OS X, there's a "__builtin_debugtrap()" that generates an int3 on x86. That seems more appropriate for this purpose anyway. Don't know whether such a thing exists on Linux, however.

Jim

[amccarth]

Restored TestBuiltinTrap.py because that's actually for something else.

Changed TestDebugbreak.py to use asm("int3") on non-Windows x86.

I've ordered a Linux box, but I don't currently have one, so I haven't been able to try this on Linux yet.

[zturner]

Windows stuff looks good to me.

[labath]

__builtin_debugtrap() indeed works (thanks Jim), but only on clang (no gcc). Since that is architecture-independent, I think we should use that and just make the test @skipIfGcc. Apart from that, the test works fine on linux after fixing the macro issue.

packages/Python/lldbsuite/test/functionalities/breakpoint/debugbreak/Makefile

6

Is anyone actually using icc? I think we should just remove that...

packages/Python/lldbsuite/test/functionalities/breakpoint/debugbreak/TestDebugBreak.py

18

I don't think these two XFAILs will be necessary now. Let's assume this will work until proven otherwise.

packages/Python/lldbsuite/test/functionalities/breakpoint/debugbreak/main.c

6

this will generate the wrong expansion (leaves () hanging). I recommend using the following:
#define BREAKPOINT_INTRINSIC() ...
to make sure the parens are consumed.

[amccarth]

Thanks for the Linux check Pavel.

I'm running one last check and then I'll submit and keep an eye on the buildbots for the rest of the day.

packages/Python/lldbsuite/test/functionalities/breakpoint/debugbreak/main.c

6

Whoops. Nice catch. Thanks.

[labath]

This was committed ages ago.

packages/Python/lldbsuite/test/functionalities/breakpoint/debugbreak/Makefile

+LEVEL = ../../../make
+
+C_SOURCES := main.c
+
+ifneq (,$(findstring icc,$(CC)))
+    CFLAGS += -debug inline-debug-info
+endif
+
+include $(LEVEL)/Makefile.rules

packages/Python/lldbsuite/test/functionalities/breakpoint/debugbreak/TestDebugBreak.py

+"""
+Test embedded breakpoints, like `asm int 3;` in x86 or or `__debugbreak` on Windows.
+"""
+
+from __future__ import print_function
+
+import os
+import lldb
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+
+class DebugBreakTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipIf(archs=not_in(["i386", "i686"]))
+    @expectedFailureAll("llvm.org/pr15936", compiler="gcc", compiler_version=["<=","4.6"])
+    @expectedFailureAll(archs="arm", compiler="gcc", triple=".*-android") # gcc generates incorrect linetable
+    @no_debug_info_test
+    def test_asm_int_3(self):
+        """Test that intrinsics like `__debugbreak();` and `__builtin_trap();` are treated like breakpoints."""
+        self.build()
+        exe = os.path.join(os.getcwd(), "a.out")
+
+        # Run the program.
+        target = self.dbg.CreateTarget(exe)
+        process = target.LaunchSimple(None, None, self.get_process_working_directory())
+
+        # We've hit the first stop, so grab the frame.
+        self.assertEqual(process.GetState(), lldb.eStateStopped)
+        thread = process.GetThreadAtIndex(0)
+        frame = thread.GetFrameAtIndex(0)
+
+        # We should be in funciton 'bar'.
+        self.assertTrue(frame.IsValid())
+        function_name = frame.GetFunctionName()
+        self.assertTrue('bar' in function_name)
+
+        # We should be able to evaluate the parameter foo.
+        value = frame.EvaluateExpression('*foo')
+        self.assertEqual(value.GetValueAsSigned(), 42)
+
+        # The counter should be 1 at the first stop and increase by 2 for each
+        # subsequent stop.
+        counter = 1
+        while counter < 20:
+          value = frame.EvaluateExpression('count')
+          self.assertEqual(value.GetValueAsSigned(), counter)
+          counter += 2
+          process.Continue()
+
+        # The inferior should exit after the last iteration.
+        self.assertEqual(process.GetState(), lldb.eStateExited)

packages/Python/lldbsuite/test/functionalities/breakpoint/debugbreak/main.c

+#ifdef _MSC_VER
+#include <intrin.h>
+#define BREAKPOINT_INTRINSIC    __debugbreak
+#else
+#define BREAKPOINT_INTRINSIC    __builtin_trap
+#endif
+
+int
+bar(int const *foo)
+{
+    int count = 0;
+    for (int i = 0; i < 10; ++i)
+    {
+        count += 1;
+        BREAKPOINT_INTRINSIC();
+        count += 1;
+    }
+    return *foo;
+}
+
+int
+main(int argc, char **argv)
+{
+    int foo = 42;
+    bar(&foo);
+    return 0;
+}
+
+

packages/Python/lldbsuite/test/linux/builtin_trap/Makefile

-LEVEL = ../../make
-
-CXX_SOURCES := main.cpp
-
-include $(LEVEL)/Makefile.rules

packages/Python/lldbsuite/test/linux/builtin_trap/TestBuiltinTrap.py

-"""
-Test lldb ability to unwind a stack with a function containing a call to the
-'__builtin_trap' intrinsic, which GCC (4.6) encodes to an illegal opcode.
-"""
-
-from __future__ import print_function
-
-
-
-import os
-import lldb
-from lldbsuite.test.lldbtest import *
-import lldbsuite.test.lldbutil as lldbutil
-
-class BuiltinTrapTestCase(TestBase):
-
-    mydir = TestBase.compute_mydir(__file__)
-
-    def setUp(self):
-        # Call super's setUp().
-        TestBase.setUp(self)
-        # Find the line number to break at.
-        self.line = line_number('main.cpp', '// Set break point at this line.')
-
-    @expectedFailureAll("llvm.org/pr15936", compiler="gcc", compiler_version=["<=","4.6"])
-    @expectedFailureAll(archs="arm", compiler="gcc", triple=".*-android") # gcc generates incorrect linetable
-    @skipIfWindows
-    def test_with_run_command(self):
-        """Test that LLDB handles a function with __builtin_trap correctly."""
-        self.build()
-        self.runCmd("file a.out", CURRENT_EXECUTABLE_SET)
-
-        lldbutil.run_break_set_by_file_and_line (self, "main.cpp", self.line,
-                                                 num_expected_locations=1,
-                                                 loc_exact=True)
-
-        self.runCmd("run", RUN_SUCCEEDED)
-
-        # The stop reason of the thread should be breakpoint.
-        self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
-            substrs = ['stopped',
-                       'stop reason = breakpoint'])
-
-        # print backtrace, expect both 'bar' and 'main' functions to be listed
-        self.expect('bt', substrs = ['bar', 'main'])
-
-        # go up one frame
-        self.runCmd("up", RUN_SUCCEEDED)
-
-        # evaluate a local
-        self.expect('p foo', substrs = ['= 5'])

packages/Python/lldbsuite/test/linux/builtin_trap/main.cpp

-//===-- main.cpp ------------------------------------------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-
-void bar(int const *foo) {
-  __builtin_trap(); // Set break point at this line.
-}
-
-int main() {
-  int foo = 5;
-  bar(&foo);
-}

source/Plugins/Process/Windows/Live/ProcessWindowsLive.cpp

ProcessWindowsLive::RefreshStateAfterStop()
     }
 
     StopInfoSP stop_info;
     m_thread_list.SetSelectedThreadByID(active_exception->GetThreadID());
     ThreadSP stop_thread = m_thread_list.GetSelectedThread();
     if (!stop_thread)
         return;
 
+    switch (active_exception->GetExceptionCode())
+    {
+        case EXCEPTION_SINGLE_STEP:
+        {
+            stop_info = StopInfo::CreateStopReasonToTrace(*stop_thread);
+            stop_thread->SetStopInfo(stop_info);
+            WINLOG_IFANY(WINDOWS_LOG_EXCEPTION | WINDOWS_LOG_STEP, "RefreshStateAfterStop single stepping thread %u",
+                         stop_thread->GetID());
+            stop_thread->SetStopInfo(stop_info);
+            return;
+        }
+
+        case EXCEPTION_BREAKPOINT:
+        {
             RegisterContextSP register_context = stop_thread->GetRegisterContext();
 
             // The current EIP is AFTER the BP opcode, which is one byte.
             uint64_t pc = register_context->GetPC() - 1;
-    if (active_exception->GetExceptionCode() == EXCEPTION_BREAKPOINT)
-    {
-        BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
 
+            BreakpointSiteSP site(GetBreakpointSiteList().FindByAddress(pc));
             if (site)
             {
                 WINLOG_IFANY(WINDOWS_LOG_BREAKPOINTS | WINDOWS_LOG_EXCEPTION,
                              "RefreshStateAfterStop detected breakpoint in process %I64u at "
                              "address 0x%I64x with breakpoint site %d",
                              m_session_data->m_debugger->GetProcess().GetProcessId(), pc, site->GetID());
 
                 if (site->ValidForThisThread(stop_thread.get()))
                 {
                     WINLOG_IFALL(WINDOWS_LOG_BREAKPOINTS | WINDOWS_LOG_EXCEPTION,
                                  "Breakpoint site %d is valid for this thread (0x%I64x), creating stop info.",
                                  site->GetID(), stop_thread->GetID());
 
                     stop_info = StopInfo::CreateStopReasonWithBreakpointSiteID(
                         *stop_thread, site->GetID());
                     register_context->SetPC(pc);
                 }
                 else
                 {
                     WINLOG_IFALL(WINDOWS_LOG_BREAKPOINTS | WINDOWS_LOG_EXCEPTION,
                                  "Breakpoint site %d is not valid for this thread, creating empty stop info.",
                                  site->GetID());
                 }
-        }
                 stop_thread->SetStopInfo(stop_info);
+                return;
             }
-    else if (active_exception->GetExceptionCode() == EXCEPTION_SINGLE_STEP)
+            else
             {
-        stop_info = StopInfo::CreateStopReasonToTrace(*stop_thread);
-        stop_thread->SetStopInfo(stop_info);
-        WINLOG_IFANY(WINDOWS_LOG_EXCEPTION | WINDOWS_LOG_STEP, "RefreshStateAfterStop single stepping thread %u",
-                     stop_thread->GetID());
+                // The thread hit a hard-coded breakpoint like an `int 3` or `__debugbreak()`.
+                WINLOG_IFALL(WINDOWS_LOG_BREAKPOINTS | WINDOWS_LOG_EXCEPTION,
+                                "No breakpoint site matches for this thread. __debugbreak()?  "
+                                "Creating stop info with the exception.");
+                // FALLTHROUGH:  We'll treat this as a generic exception record in the default case.
             }
-    else
+        }
+
+        default:
         {
             std::string desc;
             llvm::raw_string_ostream desc_stream(desc);
             desc_stream << "Exception " << llvm::format_hex(active_exception->GetExceptionCode(), 8)
-                    << " encountered at address " << llvm::format_hex(pc, 8);
+                        << " encountered at address "
+                        << llvm::format_hex(active_exception->GetExceptionAddress(), 8);
             stop_info = StopInfo::CreateStopReasonWithException(*stop_thread, desc_stream.str().c_str());
             stop_thread->SetStopInfo(stop_info);
             WINLOG_IFALL(WINDOWS_LOG_EXCEPTION, desc_stream.str().c_str());
+            return;
+        }
     }
 }
 
 bool
 ProcessWindowsLive::IsAlive()
 {
     StateType state = GetPrivateState();
     switch (state)