Use BKPT instead of UDF for arm/thumb breakpoints

[tberghammer]

Summary

Use BKPT instead of UDF for arm/thumb breakpoints

The UDF instruction is deprecated in armv7 and in case of thumb2 instructions set it don't work well together with the IT instruction.

[clayborg]

There are immediate values you can play around with inside the BKPT instruction of both ARM and Thumb versions so that the two least significant bytes are the same:

static const uint8_t g_arm_breakpoint_opcode[] = { 0x70, 0xBE, 0x20, 0xE1 };
static const uint8_t g_thumb_breakpoint_opcode[] = { 0x70, 0xBE };

Note both start with 0xBE70 when using the above definitions. This means if you guess wrong and set an ARM breakpoint in Thumb code, you will still stop and not hose your program over. I would recommend using these values just to be safe.

[tberghammer]

Update breakpoint opcodes based on review

[omjavaid]

GDB doesnt use bkpt instruction for user debugging for the reason that it interferes with jtag debug probes. I am not sure if LLDB will be ever used with a jtag probe in near future but still a jtag prob might be connected to the same hardware which is using LLDB to debug a user space application.

heres a discussion which happened some years back on similar issues:

https://sourceware.org/ml/gdb-patches/2010-01/msg00624.html

http://www.spinics.net/lists/arm-kernel/msg80476.html

I am fine with using bkpt as long as it helps us debug IT blocks and doesnt interfere with any of our currently functionality.

[tberghammer]

In D16853#347120, @omjavaid wrote:

GDB doesnt use bkpt instruction for user debugging for the reason that it interferes with jtag debug probes. I am not sure if LLDB will be ever used with a jtag probe in near future but still a jtag prob might be connected to the same hardware which is using LLDB to debug a user space application.

The change is restricted to Linux and I am pretty sure that if somebody want to do jtag debugging with LLDB then he/she have to implement a new platform module. If a separate jtag probe is connected to the hardware then it might cause an issue but I don't think we have any user who is doing it (why would you use 2 debugging at the same time?)

heres a discussion which happened some years back on similar issues:

https://sourceware.org/ml/gdb-patches/2010-01/msg00624.html

http://www.spinics.net/lists/arm-kernel/msg80476.html

If we hit the issues they are mentioning with using the BKPT instruction then we have to implement some workarounds for it but as they describe they are not perfect so I would rather avoid them.

I am fine with using bkpt as long as it helps us debug IT blocks and doesnt interfere with any of our currently functionality.

I run the test suit and nothing showed up so I think we are mostly safe in this side.

[clayborg]

Looks good. Most JTAG probes actually vend their own GDB server these days. If so, we will used the Z packets in order to set breakpoints, so we will be isolated from these issues. If we add a native JTAG Process plug-in, it will be coded to "do the right thing" and it won't re-use this code, so this code should be good to go.

[EwanCrawford]

Hi Tamas,

This commit causes a SIGBUS for me on Nexus 6 ARMv7 devices, specifically the g_thumb_breakpoint_opcode change.
I can revert this locally but any advice on tracking the issue down further?

[tberghammer]

Hi Ewan,

Can you give me some instruction about how can I reproduce the issue on a
Nexus 6? (If you can't gave the information out then the disassembly of the
offending function and the fault address might gave me some hint as well)

A SIGBUS error looks a bit strange for me but my best guess is that we
manage to install a 2 byte thumb breakpoint on the 2nd 2 byte of a 4 byte
instruction what results in an incorrect instruction opcode and from that
point strange things can happen.

Tamas

lldb/trunk/source/Plugins/Platform/Linux/PlatformLinux.cpp

case llvm::Triple::hexagon:
         {
             static const uint8_t g_hex_opcode[] = { 0x0c, 0xdb, 0x00, 0x54 };
             trap_opcode = g_hex_opcode;
             trap_opcode_size = sizeof(g_hex_opcode);
         }
         break;
     case llvm::Triple::arm:
         {
-            // The ARM reference recommends the use of 0xe7fddefe and 0xdefe
-            // but the linux kernel does otherwise.
-            static const uint8_t g_arm_breakpoint_opcode[] = { 0xf0, 0x01, 0xf0, 0xe7 };
-            static const uint8_t g_thumb_breakpoint_opcode[] = { 0x01, 0xde };
+            static const uint8_t g_arm_breakpoint_opcode[] = { 0x70, 0xbe, 0x20, 0xe1 };
+            static const uint8_t g_thumb_breakpoint_opcode[] = { 0x70, 0xbe };
 
             lldb::BreakpointLocationSP bp_loc_sp (bp_site->GetOwnerAtIndex (0));
             AddressClass addr_class = eAddressClassUnknown;
 
             if (bp_loc_sp)
             {
                 addr_class = bp_loc_sp->GetAddress ().GetAddressClass ();
 

lldb/trunk/source/Plugins/Process/Linux/NativeProcessLinux.cpp

case TRAP_HWBKPT: // We receive this on watchpoint hit
                         "pid = %" PRIu64 " error = %s",
                         __FUNCTION__, thread.GetID(), error.AsCString());
         if (wp_index != LLDB_INVALID_INDEX32)
         {
             MonitorWatchpoint(thread, wp_index);
             break;
         }
 
-        // Otherwise, report step over
-        MonitorTrace(thread);
+        if (m_arch.GetMachine() == llvm::Triple::arm)
+            MonitorBreakpoint(thread); // Arm linux reports trace for breakpoint hits
+        else
+            MonitorTrace(thread); // Report the trace
         break;
     }
 
     case SI_KERNEL:
 #if defined __mips__
         // For mips there is no special signal for watchpoint
         // So we check for watchpoint in kernel trap
     {

 Error
 NativeProcessLinux::GetSoftwareBreakpointTrapOpcode (size_t trap_opcode_size_hint,
                                                      size_t &actual_opcode_size,
                                                      const uint8_t *&trap_opcode_bytes)
 {
     // FIXME put this behind a breakpoint protocol class that can be set per
     // architecture.  Need MIPS support here.
     static const uint8_t g_aarch64_opcode[] = { 0x00, 0x00, 0x20, 0xd4 };
-    // The ARM reference recommends the use of 0xe7fddefe and 0xdefe but the
-    // linux kernel does otherwise.
-    static const uint8_t g_arm_breakpoint_opcode[] = { 0xf0, 0x01, 0xf0, 0xe7 };
+    static const uint8_t g_arm_breakpoint_opcode[] = { 0x70, 0xbe, 0x20, 0xe1 };
     static const uint8_t g_i386_opcode [] = { 0xCC };
     static const uint8_t g_mips64_opcode[] = { 0x00, 0x00, 0x00, 0x0d };
     static const uint8_t g_mips64el_opcode[] = { 0x0d, 0x00, 0x00, 0x00 };
-    static const uint8_t g_thumb_breakpoint_opcode[] = { 0x01, 0xde };
+    static const uint8_t g_thumb_breakpoint_opcode[] = { 0x70, 0xbe };
 
     switch (m_arch.GetMachine ())
     {
     case llvm::Triple::aarch64:
         trap_opcode_bytes = g_aarch64_opcode;
         actual_opcode_size = sizeof(g_aarch64_opcode);
         return Error ();