Refactor GetSoftwareBreakpointTrapOpcode

[ADodds]

Summary

This patch aims to reduce the code duplication among all of the platforms in GetSoftwareBreakpointTrapOpcode. Common code has been pushed into the Platform base class, and only special case platform code has been left in the derived platforms.

When remote debugging it can be the case that your current host platform will be queried for the trap opcode to use on the target, which currently lead to different results depending on the host lldb is running on. With this patch, platforms have a more unified view of trap opcodes for targets.

Some platforms however have specific/special case requirements so I should not have effected their functionality.

Do others think this is a usefull refactor, or have suggestions/feedback?

[emaste]

Nice work, happy to see this deduplication.

lldb/trunk/include/lldb/Target/Platform.h

//        FindPlugin (Process *process, const ConstString &plugin_name);
         ///     \b true if \a arch was filled in and is valid, \b false 
         ///     otherwise.
         //------------------------------------------------------------------
         virtual bool
         GetSupportedArchitectureAtIndex (uint32_t idx, ArchSpec &arch) = 0;
 
         virtual size_t
         GetSoftwareBreakpointTrapOpcode (Target &target,
-                                         BreakpointSite *bp_site) = 0;
+                                         BreakpointSite *bp_site);
 
         //------------------------------------------------------------------
         /// Launch a new process on a platform, not necessarily for 
         /// debugging, it could be just for running the process.
         //------------------------------------------------------------------
         virtual Error
         LaunchProcess (ProcessLaunchInfo &launch_info);
 

lldb/trunk/source/Plugins/Platform/FreeBSD/PlatformFreeBSD.cpp

 #endif
 
     Platform::GetStatus(strm);
 }
 
 size_t
 PlatformFreeBSD::GetSoftwareBreakpointTrapOpcode (Target &target, BreakpointSite *bp_site)
 {
-    ArchSpec arch = target.GetArchitecture();
-    const uint8_t *trap_opcode = NULL;
-    size_t trap_opcode_size = 0;
-
-    switch (arch.GetMachine())
-    {
-    default:
-        assert(false && "Unhandled architecture in PlatformFreeBSD::GetSoftwareBreakpointTrapOpcode()");
-        break;
-    case llvm::Triple::aarch64:
-        {
-            static const uint8_t g_aarch64_opcode[] = { 0x00, 0x00, 0x20, 0xd4 };
-            trap_opcode = g_aarch64_opcode;
-            trap_opcode_size = sizeof(g_aarch64_opcode);
-        }
-        break;
-    // TODO: support big-endian arm and thumb trap codes.
+    switch (target.GetArchitecture().GetMachine())
     case llvm::Triple::arm:
         {
-            static const uint8_t g_arm_breakpoint_opcode[] = { 0xfe, 0xde, 0xff, 0xe7 };
-            static const uint8_t g_thumb_breakpoint_opcode[] = { 0x01, 0xde };
-
-            lldb::BreakpointLocationSP bp_loc_sp (bp_site->GetOwnerAtIndex (0));
+            lldb::BreakpointLocationSP bp_loc_sp(bp_site->GetOwnerAtIndex(0));
             AddressClass addr_class = eAddressClassUnknown;
 
             if (bp_loc_sp)
-                addr_class = bp_loc_sp->GetAddress ().GetAddressClass ();
+            {
+                addr_class = bp_loc_sp->GetAddress().GetAddressClass();
+                if (addr_class == eAddressClassUnknown && (bp_loc_sp->GetAddress().GetFileAddress() & 1))
+                    addr_class = eAddressClassCodeAlternateISA;
+            }
 
-            if (addr_class == eAddressClassCodeAlternateISA
-                || (addr_class == eAddressClassUnknown && (bp_site->GetLoadAddress() & 1)))
+            if (addr_class == eAddressClassCodeAlternateISA)
             {
                 // TODO: Enable when FreeBSD supports thumb breakpoints.
                 // FreeBSD kernel as of 10.x, does not support thumb breakpoints
-                trap_opcode = g_thumb_breakpoint_opcode;
-                trap_opcode_size = 0;
-            }
-            else
-            {
-                trap_opcode = g_arm_breakpoint_opcode;
-                trap_opcode_size = sizeof(g_arm_breakpoint_opcode);
-            }
-        }
-        break;
-    case llvm::Triple::mips64:
-        {
-            static const uint8_t g_hex_opcode[] = { 0x00, 0x00, 0x00, 0x0d };
-            trap_opcode = g_hex_opcode;
-            trap_opcode_size = sizeof(g_hex_opcode);
-        }
-        break;
-    case llvm::Triple::mips64el:
-        {
-            static const uint8_t g_hex_opcode[] = { 0x0d, 0x00, 0x00, 0x00 };
-            trap_opcode = g_hex_opcode;
-            trap_opcode_size = sizeof(g_hex_opcode);
-        }
-        break;
-    case llvm::Triple::ppc:
-    case llvm::Triple::ppc64:
-        {
-            static const uint8_t g_ppc_opcode[] = { 0x7f, 0xe0, 0x00, 0x08 };
-            trap_opcode = g_ppc_opcode;
-            trap_opcode_size = sizeof(g_ppc_opcode);
-        }
-        break;
-    case llvm::Triple::x86:
-    case llvm::Triple::x86_64:
-        {
-            static const uint8_t g_i386_opcode[] = { 0xCC };
-            trap_opcode = g_i386_opcode;
-            trap_opcode_size = sizeof(g_i386_opcode);
+                return 0;
             }
-        break;
         }
 
-    if (bp_site->SetTrapOpcode(trap_opcode, trap_opcode_size))
-        return trap_opcode_size;
-    return 0;
+        // Fall through...
+    default:
+        return Platform::GetSoftwareBreakpointTrapOpcode(target, bp_site);
+    }
 }
 
 
 void
 PlatformFreeBSD::CalculateTrapHandlerSymbolNames ()
 {
     m_trap_handlers.push_back (ConstString ("_sigtramp"));
 }

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

public:
 
         uint32_t
         FindProcesses (const ProcessInstanceInfoMatch &match_info,
                        ProcessInstanceInfoList &process_infos) override;
 
         bool
         GetSupportedArchitectureAtIndex (uint32_t idx, ArchSpec &arch) override;
 
-        size_t
-        GetSoftwareBreakpointTrapOpcode (Target &target,
-                                         BreakpointSite *bp_site) override;
-
         int32_t
         GetResumeCountForLaunchInfo (ProcessLaunchInfo &launch_info) override;
 
         bool
         CanDebugProcess () override;
 
         lldb::ProcessSP
         DebugProcess (ProcessLaunchInfo &launch_info,

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

if (IsHost())
 
         strm.Printf ("    Kernel: %s\n", un.sysname);
         strm.Printf ("   Release: %s\n", un.release);
         strm.Printf ("   Version: %s\n", un.version);
     }
 #endif
 }
 
-size_t
-PlatformLinux::GetSoftwareBreakpointTrapOpcode (Target &target,
-                                                BreakpointSite *bp_site)
-{
-    ArchSpec arch = target.GetArchitecture();
-    const uint8_t *trap_opcode = NULL;
-    size_t trap_opcode_size = 0;
-
-    switch (arch.GetMachine())
-    {
-    default:
-        assert(false && "CPU type not supported!");
-        break;
-
-    case llvm::Triple::aarch64:
-        {
-            static const uint8_t g_aarch64_opcode[] = { 0x00, 0x00, 0x20, 0xd4 };
-            trap_opcode = g_aarch64_opcode;
-            trap_opcode_size = sizeof(g_aarch64_opcode);
-        }
-        break;
-    case llvm::Triple::x86:
-    case llvm::Triple::x86_64:
-        {
-            static const uint8_t g_i386_breakpoint_opcode[] = { 0xCC };
-            trap_opcode = g_i386_breakpoint_opcode;
-            trap_opcode_size = sizeof(g_i386_breakpoint_opcode);
-        }
-        break;
-    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 };
-
-            lldb::BreakpointLocationSP bp_loc_sp (bp_site->GetOwnerAtIndex (0));
-            AddressClass addr_class = eAddressClassUnknown;
-
-            if (bp_loc_sp)
-            {
-                addr_class = bp_loc_sp->GetAddress ().GetAddressClass ();
-
-                if (addr_class == eAddressClassUnknown &&
-                    (bp_loc_sp->GetAddress ().GetFileAddress () & 1))
-                {
-                    addr_class = eAddressClassCodeAlternateISA;
-                }
-            }
-
-            if (addr_class == eAddressClassCodeAlternateISA)
-            {
-                trap_opcode = g_thumb_breakpoint_opcode;
-                trap_opcode_size = sizeof(g_thumb_breakpoint_opcode);
-            }
-            else
-            {
-                trap_opcode = g_arm_breakpoint_opcode;
-                trap_opcode_size = sizeof(g_arm_breakpoint_opcode);
-            }
-        }
-        break;
-    case llvm::Triple::mips:
-    case llvm::Triple::mips64:
-        {
-            static const uint8_t g_hex_opcode[] = { 0x00, 0x00, 0x00, 0x0d };
-            trap_opcode = g_hex_opcode;
-            trap_opcode_size = sizeof(g_hex_opcode);
-        }
-        break;
-    case llvm::Triple::mipsel:
-    case llvm::Triple::mips64el:
-        {
-            static const uint8_t g_hex_opcode[] = { 0x0d, 0x00, 0x00, 0x00 };
-            trap_opcode = g_hex_opcode;
-            trap_opcode_size = sizeof(g_hex_opcode);
-        }
-        break;
-    }
-
-    if (bp_site->SetTrapOpcode(trap_opcode, trap_opcode_size))
-        return trap_opcode_size;
-    return 0;
-}
-
 int32_t
 PlatformLinux::GetResumeCountForLaunchInfo (ProcessLaunchInfo &launch_info)
 {
     int32_t resume_count = 0;
 
     // Always resume past the initial stop when we use eLaunchFlagDebug
     if (launch_info.GetFlags ().Test (eLaunchFlagDebug))
     {

lldb/trunk/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp

PlatformDarwin::GetSharedModule (const ModuleSpec &module_spec,
     if (module_sp)
         module_sp->SetPlatformFileSpec(module_spec.GetFileSpec());
     return error;
 }
 
 size_t
 PlatformDarwin::GetSoftwareBreakpointTrapOpcode (Target &target, BreakpointSite *bp_site)
 {
-    const uint8_t *trap_opcode = NULL;
+    const uint8_t *trap_opcode = nullptr;
     uint32_t trap_opcode_size = 0;
     bool bp_is_thumb = false;
-        
     llvm::Triple::ArchType machine = target.GetArchitecture().GetMachine();
     switch (machine)
     {
-    case llvm::Triple::x86:
-    case llvm::Triple::x86_64:
-        {
-            static const uint8_t g_i386_breakpoint_opcode[] = { 0xCC };
-            trap_opcode = g_i386_breakpoint_opcode;
-            trap_opcode_size = sizeof(g_i386_breakpoint_opcode);
-        }
-        break;
-
     case llvm::Triple::aarch64:
         {
             // TODO: fix this with actual darwin breakpoint opcode for arm64.
             // right now debugging uses the Z packets with GDB remote so this
             // is not needed, but the size needs to be correct...
             static const uint8_t g_arm64_breakpoint_opcode[] = { 0xFE, 0xDE, 0xFF, 0xE7 };
             trap_opcode = g_arm64_breakpoint_opcode;
             trap_opcode_size = sizeof(g_arm64_breakpoint_opcode);
case llvm::Triple::arm:
                 trap_opcode = g_thumb_breakpooint_opcode;
                 trap_opcode_size = sizeof(g_thumb_breakpooint_opcode);
                 break;
             }
             trap_opcode = g_arm_breakpoint_opcode;
             trap_opcode_size = sizeof(g_arm_breakpoint_opcode);
         }
         break;
-        
     case llvm::Triple::ppc:
     case llvm::Triple::ppc64:
         {
             static const uint8_t g_ppc_breakpoint_opcode[] = { 0x7F, 0xC0, 0x00, 0x08 };
             trap_opcode = g_ppc_breakpoint_opcode;
             trap_opcode_size = sizeof(g_ppc_breakpoint_opcode);
         }
         break;
-        
     default:
-        assert(!"Unhandled architecture in PlatformDarwin::GetSoftwareBreakpointTrapOpcode()");
-        break;
+        return Platform::GetSoftwareBreakpointTrapOpcode(target, bp_site);
     }
-    
     if (trap_opcode && trap_opcode_size)
     {
         if (bp_site->SetTrapOpcode(trap_opcode, trap_opcode_size))
             return trap_opcode_size;
     }
     return 0;
 
 }

lldb/trunk/source/Plugins/Platform/NetBSD/PlatformNetBSD.h

public:
                         std::string *command_output,
                         uint32_t timeout_sec) override;
 
         Error
         ResolveExecutable(const ModuleSpec &module_spec,
                           lldb::ModuleSP &module_sp,
                           const FileSpecList *module_search_paths_ptr) override;
 
-        size_t
-        GetSoftwareBreakpointTrapOpcode(Target &target,
-                                        BreakpointSite *bp_site) override;
-
         bool
         GetRemoteOSVersion () override;
 
         bool
         GetRemoteOSBuildString (std::string &s) override;
 
         bool
         GetRemoteOSKernelDescription (std::string &s) override;

lldb/trunk/source/Plugins/Platform/NetBSD/PlatformNetBSD.cpp

if (::uname(&un) == -1) {
         // Dump a common information about the platform status.
         strm << "Host: " << un.sysname << ' ' << un.release << ' ' << un.version << '\n';
     }
 #endif
 
     Platform::GetStatus(strm);
 }
 
-size_t
-PlatformNetBSD::GetSoftwareBreakpointTrapOpcode (Target &target, BreakpointSite *bp_site)
-{
-    ArchSpec arch = target.GetArchitecture();
-    const uint8_t *trap_opcode = NULL;
-    size_t trap_opcode_size = 0;
-
-    switch (arch.GetMachine())
-    {
-    default:
-        assert(false && "Unhandled architecture in PlatformNetBSD::GetSoftwareBreakpointTrapOpcode()");
-        break;
-    case llvm::Triple::x86:
-    case llvm::Triple::x86_64:
-        {
-            static const uint8_t g_i386_opcode[] = { 0xCC };
-            trap_opcode = g_i386_opcode;
-            trap_opcode_size = sizeof(g_i386_opcode);
-        }
-        break;
-    }
-
-    if (bp_site->SetTrapOpcode(trap_opcode, trap_opcode_size))
-        return trap_opcode_size;
-    return 0;
-}
-
-
 void
 PlatformNetBSD::CalculateTrapHandlerSymbolNames ()
 {
     m_trap_handlers.push_back (ConstString ("_sigtramp"));
 }
 
 Error
 PlatformNetBSD::LaunchProcess (ProcessLaunchInfo &launch_info)

lldb/trunk/source/Plugins/Platform/Windows/PlatformWindows.h

ResolveExecutable(const lldb_private::ModuleSpec &module_spec,
                       const FileSpecList *module_search_paths_ptr) override;
 
     const char *
     GetDescription() override
     {
         return GetPluginDescriptionStatic(IsHost());
     }
 
-    size_t
-    GetSoftwareBreakpointTrapOpcode(lldb_private::Target &target,
-                                    lldb_private::BreakpointSite *bp_site) override;
-
     bool
     GetRemoteOSVersion() override;
 
     bool
     GetRemoteOSBuildString(std::string &s) override;
 
     bool
     GetRemoteOSKernelDescription(std::string &s) override;

lldb/trunk/source/Plugins/Platform/Windows/PlatformWindows.cpp

if (error.Success())
                 }
             }
         }
     }
 
     return error;
 }
 
-size_t
-PlatformWindows::GetSoftwareBreakpointTrapOpcode (Target &target, BreakpointSite *bp_site)
-{
-    ArchSpec arch = target.GetArchitecture();
-    const uint8_t *trap_opcode = nullptr;
-    size_t trap_opcode_size = 0;
-
-    switch (arch.GetMachine())
-    {
-    case llvm::Triple::x86:
-    case llvm::Triple::x86_64:
-        {
-            static const uint8_t g_i386_opcode[] = { 0xCC };
-            trap_opcode = g_i386_opcode;
-            trap_opcode_size = sizeof(g_i386_opcode);
-        }
-        break;
-
-    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;
-    default:
-        llvm_unreachable("Unhandled architecture in PlatformWindows::GetSoftwareBreakpointTrapOpcode()");
-        break;
-    }
-
-    if (bp_site->SetTrapOpcode(trap_opcode, trap_opcode_size))
-        return trap_opcode_size;
-
-    return 0;
-}
-
 bool
 PlatformWindows::GetRemoteOSVersion ()
 {
     if (m_remote_platform_sp)
         return m_remote_platform_sp->GetOSVersion (m_major_os_version,
                                                    m_minor_os_version,
                                                    m_update_os_version);
     return false;

lldb/trunk/source/Target/Platform.cpp

 
 // Other libraries and framework includes
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
 // Project includes
 #include "lldb/Target/Platform.h"
 #include "lldb/Breakpoint/BreakpointIDList.h"
+#include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/DataBufferHeap.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Error.h"
 #include "lldb/Core/Log.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/StreamFile.h"

 }
 
 size_t
 Platform::ConnectToWaitingProcesses(lldb_private::Debugger& debugger, lldb_private::Error& error)
 {
     error.Clear();
     return 0;
 }
+
+size_t
+Platform::GetSoftwareBreakpointTrapOpcode(Target &target, BreakpointSite *bp_site)
+{
+    ArchSpec arch = target.GetArchitecture();
+    const uint8_t *trap_opcode = nullptr;
+    size_t trap_opcode_size = 0;
+
+    switch (arch.GetMachine())
+    {
+    case llvm::Triple::aarch64:
+        {
+            static const uint8_t g_aarch64_opcode[] = {0x00, 0x00, 0x20, 0xd4};
+            trap_opcode = g_aarch64_opcode;
+            trap_opcode_size = sizeof(g_aarch64_opcode);
+        }
+        break;
+
+    // TODO: support big-endian arm and thumb trap codes.
+    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};
+
+            lldb::BreakpointLocationSP bp_loc_sp(bp_site->GetOwnerAtIndex(0));
+            AddressClass addr_class = eAddressClassUnknown;
+
+            if (bp_loc_sp)
+            {
+                addr_class = bp_loc_sp->GetAddress().GetAddressClass();
+                if (addr_class == eAddressClassUnknown && (bp_loc_sp->GetAddress().GetFileAddress() & 1))
+                    addr_class = eAddressClassCodeAlternateISA;
+            }
+
+            if (addr_class == eAddressClassCodeAlternateISA)
+            {
+                trap_opcode = g_thumb_breakpoint_opcode;
+                trap_opcode_size = sizeof(g_thumb_breakpoint_opcode);
+            }
+            else
+            {
+                trap_opcode = g_arm_breakpoint_opcode;
+                trap_opcode_size = sizeof(g_arm_breakpoint_opcode);
+            }
+        }
+        break;
+
+    case llvm::Triple::mips64:
+        {
+            static const uint8_t g_hex_opcode[] = {0x00, 0x00, 0x00, 0x0d};
+            trap_opcode = g_hex_opcode;
+            trap_opcode_size = sizeof(g_hex_opcode);
+        }
+        break;
+
+    case llvm::Triple::mips64el:
+        {
+            static const uint8_t g_hex_opcode[] = {0x0d, 0x00, 0x00, 0x00};
+            trap_opcode = g_hex_opcode;
+            trap_opcode_size = sizeof(g_hex_opcode);
+        }
+        break;
+
+    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::ppc:
+    case llvm::Triple::ppc64:
+        {
+            static const uint8_t g_ppc_opcode[] = {0x7f, 0xe0, 0x00, 0x08};
+            trap_opcode = g_ppc_opcode;
+            trap_opcode_size = sizeof(g_ppc_opcode);
+        }
+        break;
+
+    case llvm::Triple::x86:
+    case llvm::Triple::x86_64:
+        {
+            static const uint8_t g_i386_opcode[] = {0xCC};
+            trap_opcode = g_i386_opcode;
+            trap_opcode_size = sizeof(g_i386_opcode);
+        }
+        break;
+
+    default:
+        assert(!"Unhandled architecture in Platform::GetSoftwareBreakpointTrapOpcode");
+        break;
+    }
+
+    assert(bp_site);
+    if (bp_site->SetTrapOpcode(trap_opcode, trap_opcode_size))
+        return trap_opcode_size;
+
+    return 0;
+}