Move architecture-specific address adjustment to architecture plugins.

[tatyana-krasnukha]

Summary

Was discussed in "[lldb-dev] Handling callable addresses" thread.

[tatyana-krasnukha]

Removed unrelated changes from the patch.

[tatyana-krasnukha]

I doubt that I should create plugin for thumb also. Cannot ArchitectureArm handle it?
And what about big-endian versions?

source/Plugins/Architecture/Mips/ArchitectureMips.cpp

39

Is it ok to have single architecture plugin for all these machines?

source/Target/Target.cpp

2353

Supposed this is fair regardless of architecture.

[labath]

include/lldb/Core/Architecture.h

74–76

The rest of the codebase doesn't use the /*commented_out_argument*/ style.

source/Plugins/Architecture/Mips/ArchitectureMips.cpp

38–42

Use ArchSpec::IsMIPS() ?

source/Target/Target.cpp

2353

Why do we need the DoesSupport call? Could we just have the default implementation of GetAddressFlagMask return 0?

[labath]

In D48623#1144543, @tatyana-krasnukha wrote:

I doubt that I should create plugin for thumb also. Cannot ArchitectureArm handle it?

I don't think a special thumb plugin would be good, as you can have mixed arm&thumb code within a single process.

And what about big-endian versions?

Let's try to avoid them for now. Hopefully the differences (if any) can be handled by simple helper functions.

source/Plugins/Architecture/Mips/ArchitectureMips.cpp

39

I think that is fine for the time being. We can always revisit that if the implementations start to diverge.

[tatyana-krasnukha]

source/Target/Target.cpp

2353

If target doesn't support this trick with special bits in address, it may need aligned callable addresses as well. In this case we fall back to GetOpcodeLoadAddress.

[labath]

source/Target/Target.cpp

2353

Hmm... why is that fallback there in the first place? I don't see anything like that in the original code. I thought this was meant to be NFC.

[tatyana-krasnukha]

source/Target/Target.cpp

2353

If user executes 'thread jump' command with unaligned address, last will be written to PC register as is and I'm not sure that things will be good after that. May be over-caution.

[tatyana-krasnukha]

Uncommented arguments and used ArchSpec::IsMIPS()

[clayborg]

Instead of adding a bunch of calls to the Architecture for feature flags and masks and instructions, just ask the architecture plug-in to do the work you need them to do. See inlined comments,

include/lldb/Core/Architecture.h

17–18

#include lldb-forward.h for this kind of stuff.

70–71

Remove Feature and DoesSupport(). Just add GetCallableLoadAddress(...) and GetBreakableLoadAddress(...) to the architecture plug-in.

73–78

Remove. See above comment.

80–83

Remove, see above comments

source/Target/Target.cpp

383

revert

389

revert

2350–2351

Just let architecture plugin do this if needed inside Architecture::GetCallableLoadAddress(...)

2350–2351

Just add a Architecture::GetBreakableLoadAddress() and remove all arch specific functionality in this fucntion.

2353

Add a "GetCallableLoadAddress(...)" function to the Architecture plug-in. Remove the Architecture::GetAddressFlagMask().

[tatyana-krasnukha]

Upd: Making functions static with Target* parameter allowed to rid out of checking target and architecture plugin by caller code.

[clayborg]

Great patch, but I would still rather ask the target to do this for us rather than having a static function on a different class that must take a Target as the first argument.

include/lldb/Core/Architecture.h

82

remove static from all these and make virtual

108–120

Remove all these Do calls

[tatyana-krasnukha]

@clayborg , thank you for the feedback, but why don't you like the approach with static functions?

I see a few advantages in it:

• no need to duplicate code that is common for all architectures; from the other hand, no need to call base class's function from overridden one (that is easy to forget)
• function call

auto code_addr = Architecture::GetCallableLoadAddress(target_ptr, addr, AddressClass::eCode);

looks more readable against

 addr_t code_addr = addr;
 if (target_ptr) {  
    auto arch = target->GetArchitecturePlugin();
    if (arch)
        code_addr = arch->GetCallableLoadAddress(addr, AddressClass::eCode);
}

and is safer, because there are places where caller doesn't check pointers.

• and finally, it is consistent with existing interface of Address class, whose GetCallableLoadAddress and GetOpcodeLoadAddress take a Target as the first argument as well.

Also, GetBreakableLoadAddress needs a Target anyway.

I would be grateful if you could point where my thoughts are wrong.

source/Core/Address.cpp

356

unsafe

source/Target/RegisterContext.cpp

137

too safe;)

[clayborg]

In D48623#1168030, @tatyana-krasnukha wrote:

@clayborg , thank you for the feedback, but why don't you like the approach with static functions?

I see a few advantages in it:

• no need to duplicate code that is common for all architectures; from the other hand, no need to call base class's function from overridden one (that is easy to forget)
• function call

auto code_addr = Architecture::GetCallableLoadAddress(target_ptr, addr, AddressClass::eCode);

looks more readable against

 addr_t code_addr = addr;
 if (target_ptr) {  
    auto arch = target->GetArchitecturePlugin();
    if (arch)
        code_addr = arch->GetCallableLoadAddress(addr, AddressClass::eCode);
}

This code would actually be:

addr_t code_addr = addr;
if (target_ptr) 
  code_addr = target->GetCallableLoadAddress(addr, AddressClass::eCode);

Any code that is still checking both the target and the arch as you above example was doing should just be asking the target to do this, we can add functions to lldb_private::Target if needed.

and is safer, because there are places where caller doesn't check pointers.

Callers should always be checking pointers.

• and finally, it is consistent with existing interface of Address class, whose GetCallableLoadAddress and GetOpcodeLoadAddress take a Target as the first argument as well.

The address object is still being asked do something:

addr.GetCallableLoadAddress(target, ...)

Not:

Address::GetCallableLoadAddress(addr, target, ...);

Also, GetBreakableLoadAddress needs a Target anyway.

I would be grateful if you could point where my thoughts are wrong.

I just like to keep this object oriented by asking the object we want to do something for us. Doing the static function solution seems like a C kind of thing that is being done in C++

[tatyana-krasnukha]

Updated according to comments, sorry for long delay.

[tatyana-krasnukha]

Make overriding functions public.

[clayborg]

So I would suggest not removing any functions from lldb_private::Target, just change the old ones to call through the arch plug-ins if there is one. Then many changes in this diff just go away and we still get the clean implementation where things are delegated to the Architecture plug-ins

include/lldb/Target/Target.h

-38–1

Why did we remove these calls from target? We should have default implementations that do nothing if there is no arch plug-in, and call through the arch plug-in if there is one. As I mentioned we don't want people to have to check if the arch plug-in is valid at each call site.

source/Core/Address.cpp

-4–1

None of the changes in Address.cpp are needed if we leave the functions in Target.

source/Target/RegisterContext.cpp

-4–4

None of these changes are needed if we leave the function in Target.h/.cpp

source/Target/Target.cpp

-248–1

None of these changes are needed if we leave the function in Target.h/.cpp

0–1

Leave this function in and call through to arch plug-in if we have one.

0–1

Leave this function in and call through to arch plug-in if we have one.

0–1

Leave this function in and call through to arch plug-in if we have one.

source/Target/ThreadPlanRunToAddress.cpp

-1–1

None of these changes are needed if we leave the function in Target.h/.cpp

0–2

None of these changes are needed if we leave the function in Target.h/.cpp

[clayborg]

much better! Thanks for making the changes

[tatyana-krasnukha]

Thanks for your patience;)

include/lldb/Core/Architecture.h

 //===-- Architecture.h ------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_CORE_ARCHITECTURE_H
 #define LLDB_CORE_ARCHITECTURE_H
 
 #include "lldb/Core/PluginInterface.h"
 
 namespace lldb_private {
 
 class Architecture : public PluginInterface {
 public:
   Architecture() = default;
   virtual ~Architecture() = default;
 
   //------------------------------------------------------------------
   /// This is currently intended to handle cases where a
   /// program stops at an instruction that won't get executed and it
   /// allows the stop reason, like "breakpoint hit", to be replaced
   /// with a different stop reason like "no stop reason".
public:
   /// This is specifically used for PPC64, for functions that have
   /// both a global and a local entry point. In this case, the
   /// breakpoint is adjusted to the first function address reached
   /// by both entry points.
   //------------------------------------------------------------------
   virtual void AdjustBreakpointAddress(const Symbol &func,
                                        Address &addr) const {}
 
+
+  //------------------------------------------------------------------
+  /// Get \a load_addr as a callable code load address for this target
+  ///
+  /// Take \a load_addr and potentially add any address bits that are
+  /// needed to make the address callable. For ARM this can set bit
+  /// zero (if it already isn't) if \a load_addr is a thumb function.
+  /// If \a addr_class is set to AddressClass::eInvalid, then the address
+  /// adjustment will always happen. If it is set to an address class
+  /// that doesn't have code in it, LLDB_INVALID_ADDRESS will be
+  /// returned.
+  //------------------------------------------------------------------
+  static lldb::addr_t GetCallableLoadAddress(Target *target,
+      lldb::addr_t addr, AddressClass addr_class = AddressClass::eInvalid);
+
+  //------------------------------------------------------------------
+  /// Get \a load_addr as an opcode for this target.
+  ///
+  /// Take \a load_addr and potentially strip any address bits that are
+  /// needed to make the address point to an opcode. For ARM this can
+  /// clear bit zero (if it already isn't) if \a load_addr is a
+  /// thumb function and load_addr is in code.
+  /// If \a addr_class is set to AddressClass::eInvalid, then the address
+  /// adjustment will always happen. If it is set to an address class
+  /// that doesn't have code in it, LLDB_INVALID_ADDRESS will be
+  /// returned.
+  //------------------------------------------------------------------
+  static lldb::addr_t GetOpcodeLoadAddress(Target *target,
+      lldb::addr_t addr, AddressClass addr_class = AddressClass::eInvalid);
+
+  // Get load_addr as breakable load address for this target. Take a addr and
+  // check if for any reason there is a better address than this to put a
+  // breakpoint on. If there is then return that address. For MIPS, if
+  // instruction at addr is a delay slot instruction then this method will find
+  // the address of its previous instruction and return that address.
+  static lldb::addr_t GetBreakableLoadAddress(Target *, lldb::addr_t addr);
+
 private:
+  virtual lldb::addr_t DoGetCallableLoadAddress(lldb::addr_t addr,
+                                                bool is_alternate_isa) const {
+    return addr;
+  }
+
+  virtual lldb::addr_t DoGetOpcodeLoadAddress(lldb::addr_t addr) const {
+    return addr;
+  }
+
+  virtual lldb::addr_t DoGetBreakableLoadAddress(lldb::addr_t addr,
+                                                 Target &) const {
+    return addr;
+  }
+
   Architecture(const Architecture &) = delete;
   void operator=(const Architecture &) = delete;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_CORE_ARCHITECTURE_H

include/lldb/Target/Target.h

 //===-- Target.h ------------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
void ApplyNameToBreakpoints(BreakpointName &bp_name);
 
   Status CreateBreakpointsFromFile(const FileSpec &file,
                                    BreakpointIDList &new_bps);
 
   Status CreateBreakpointsFromFile(const FileSpec &file,
                                    std::vector<std::string> &names,
                                    BreakpointIDList &new_bps);
 
-  //------------------------------------------------------------------
-  /// Get \a load_addr as a callable code load address for this target
-  ///
-  /// Take \a load_addr and potentially add any address bits that are
-  /// needed to make the address callable. For ARM this can set bit
-  /// zero (if it already isn't) if \a load_addr is a thumb function.
-  /// If \a addr_class is set to AddressClass::eInvalid, then the address
-  /// adjustment will always happen. If it is set to an address class
-  /// that doesn't have code in it, LLDB_INVALID_ADDRESS will be
-  /// returned.
-  //------------------------------------------------------------------
-  lldb::addr_t GetCallableLoadAddress(
-      lldb::addr_t load_addr,
-      AddressClass addr_class = AddressClass::eInvalid) const;
-
-  //------------------------------------------------------------------
-  /// Get \a load_addr as an opcode for this target.
-  ///
-  /// Take \a load_addr and potentially strip any address bits that are
-  /// needed to make the address point to an opcode. For ARM this can
-  /// clear bit zero (if it already isn't) if \a load_addr is a
-  /// thumb function and load_addr is in code.
-  /// If \a addr_class is set to AddressClass::eInvalid, then the address
-  /// adjustment will always happen. If it is set to an address class
-  /// that doesn't have code in it, LLDB_INVALID_ADDRESS will be
-  /// returned.
-  //------------------------------------------------------------------
-  lldb::addr_t GetOpcodeLoadAddress(
-      lldb::addr_t load_addr,
-      AddressClass addr_class = AddressClass::eInvalid) const;
-
-  // Get load_addr as breakable load address for this target. Take a addr and
-  // check if for any reason there is a better address than this to put a
-  // breakpoint on. If there is then return that address. For MIPS, if
-  // instruction at addr is a delay slot instruction then this method will find
-  // the address of its previous instruction and return that address.
-  lldb::addr_t GetBreakableLoadAddress(lldb::addr_t addr);
-
   void ModulesDidLoad(ModuleList &module_list);
 
   void ModulesDidUnload(ModuleList &module_list, bool delete_locations);
 
   void SymbolsDidLoad(ModuleList &module_list);
 
   void ClearModules(bool delete_locations);
 

source/Core/Address.cpp

 //===-- Address.cpp ---------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
if (is_indirect && target) {
     }
   } else {
     code_addr = GetLoadAddress(target);
   }
 
   if (code_addr == LLDB_INVALID_ADDRESS)
     return code_addr;
 
-  if (target)
-    return target->GetCallableLoadAddress(code_addr, GetAddressClass());
-  return code_addr;
+  Architecture::GetCallableLoadAddress(target, code_addr, GetAddressClass());
 }
 
 bool Address::SetCallableLoadAddress(lldb::addr_t load_addr, Target *target) {
-  if (SetLoadAddress(load_addr, target)) {
-    if (target)
-      m_offset = target->GetCallableLoadAddress(m_offset, GetAddressClass());
-    return true;
-  }
+  if (!SetLoadAddress(load_addr, target))
     return false;
+
+  m_offset = Architecture::GetCallableLoadAddress(
+      target, m_offset, GetAddressClass());
+  return true;
 }
 
 addr_t Address::GetOpcodeLoadAddress(Target *target,
                                      AddressClass addr_class) const {
   addr_t code_addr = GetLoadAddress(target);
-  if (code_addr != LLDB_INVALID_ADDRESS) {
-    if (addr_class == AddressClass::eInvalid)
-      addr_class = GetAddressClass();
-    code_addr = target->GetOpcodeLoadAddress(code_addr, addr_class);
-  }
-  return code_addr;
+  if (code_addr == LLDB_INVALID_ADDRESS) {
+    return code_addr;
+
+  return Architecture::GetOpcodeLoadAddress(target, code_addr, addr_class);
 }
 
 bool Address::SetOpcodeLoadAddress(lldb::addr_t load_addr, Target *target,
                                    AddressClass addr_class,
                                    bool allow_section_end) {
-  if (SetLoadAddress(load_addr, target, allow_section_end)) {
-    if (target) {
-      if (addr_class == AddressClass::eInvalid)
-        addr_class = GetAddressClass();
-      m_offset = target->GetOpcodeLoadAddress(m_offset, addr_class);
-    }
-    return true;
-  }
-  return false;
-}
+  if (!SetLoadAddress(load_addr, target, allow_section_end))
+    return false;
+
+  m_offset = Architecture::GetOpcodeLoadAddress(target, m_offset, addr_class);
 
 bool Address::Dump(Stream *s, ExecutionContextScope *exe_scope, DumpStyle style,
                    DumpStyle fallback_style, uint32_t addr_size) const {
   // If the section was nullptr, only load address is going to work unless we
   // are trying to deref a pointer
   SectionSP section_sp(GetSection());
   if (!section_sp && style != DumpStyleResolvedPointerDescription)
     style = DumpStyleLoadAddress;

source/Core/Architecture.cpp

+//===-- Architecture.cpp ---------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Core/Architecture.h"
+#include "lldb/Target/Target.h"
+
+namespace lldb_private {
+lldb::addr_t Architecture::GetCallableLoadAddress(Target *target,
+    lldb::addr_t code_addr, AddressClass addr_class) {
+  bool alternate_isa = false;
+
+  switch (addr_class) {
+  case AddressClass::eData:
+  case AddressClass::eDebug:
+    return LLDB_INVALID_ADDRESS;
+  case AddressClass::eCodeAlternateISA:
+    alternate_isa = true;
+    break;
+  default: break;
+  }
+
+  auto arch = target->GetArchitecturePlugin();
+  return nullptr == arch || nullptr == target ?
+      code_addr : arch->DoGetCallableLoadAddress(code_addr, alternate_isa);
+}
+
+lldb::addr_t Architecture::GetOpcodeLoadAddress(Target *target,
+    lldb::addr_t opcode_addr, AddressClass addr_class) {
+  switch (addr_class) {
+  case AddressClass::eData:
+  case AddressClass::eDebug:
+    return LLDB_INVALID_ADDRESS;
+  default: break;
+  }
+
+  auto arch = target->GetArchitecturePlugin();
+  return nullptr == arch || nullptr == target ?
+      opcode_addr : arch->DoGetOpcodeLoadAddress(opcode_addr);
+}
+
+lldb::addr_t Architecture::GetBreakableLoadAddress(Target *target,
+    lldb::addr_t brk_addr) {
+  auto arch = target->GetArchitecturePlugin();
+  return nullptr == arch || nullptr == target ?
+      brk_addr : arch->DoGetBreakableLoadAddress(brk_addr, *target);
+}
+} // namespace lldb_private
 No newline at end of file

source/Core/CMakeLists.txt

 set(LLDB_CURSES_LIBS)
 
 if (NOT LLDB_DISABLE_CURSES)
   list(APPEND LLDB_CURSES_LIBS ${CURSES_LIBRARIES})
   if(LLVM_ENABLE_TERMINFO AND HAVE_TERMINFO)
     list(APPEND LLDB_CURSES_LIBS ${TERMINFO_LIBS})
   endif()
 endif()
 
 add_lldb_library(lldbCore
   Address.cpp
   AddressRange.cpp
   AddressResolver.cpp
   AddressResolverFileLine.cpp
   AddressResolverName.cpp
+  Architecture.cpp
   Broadcaster.cpp
   Communication.cpp
   Debugger.cpp
   Disassembler.cpp
   DumpDataExtractor.cpp
   DynamicLoader.cpp
   EmulateInstruction.cpp
   Event.cpp

source/Plugins/Architecture/Arm/ArchitectureArm.h

public:
   static void Terminate();
 
   ConstString GetPluginName() override;
   uint32_t GetPluginVersion() override;
 
   void OverrideStopInfo(Thread &thread) const override;
 
 private:
+  lldb::addr_t DoGetCallableLoadAddress(lldb::addr_t load_addr,
+                                        bool is_alternate_isa) const override;
+
+  lldb::addr_t DoGetOpcodeLoadAddress(lldb::addr_t load_addr) const override;
+
   static std::unique_ptr<Architecture> Create(const ArchSpec &arch);
   ArchitectureArm() = default;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_PLUGIN_ARCHITECTURE_ARM_H

source/Plugins/Architecture/Arm/ArchitectureArm.cpp

if (ITSTATE != 0) {
         // We ARE stopped in a Thumb IT instruction on an instruction whose
         // condition doesn't pass so this instruction won't get executed.
         // Regardless of why it stopped, we need to clear the stop info
         thread.SetStopInfo(StopInfoSP());
       }
     }
   }
 }
+
+addr_t ArchitectureArm::DoGetCallableLoadAddress(addr_t code_addr,
+                                                 bool is_alternate_isa) const {
+  if ((code_addr & 2u) || is_alternate_isa)
+    return code_addr | 1u;
+  return code_addr;
+}
+
+addr_t ArchitectureArm::DoGetOpcodeLoadAddress(addr_t opcode_addr) const {
+  return opcode_addr & ~(1ull);
+}
 No newline at end of file

source/Plugins/Architecture/CMakeLists.txt

 add_subdirectory(Arm)
+add_subdirectory(Mips)
 add_subdirectory(PPC64)

source/Plugins/Architecture/Mips/ArchitectureMips.h

+//===-- ArchitectureMips.h ---------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_PLUGIN_ARCHITECTURE_MIPS_H
+#define LLDB_PLUGIN_ARCHITECTURE_MIPS_H
+
+#include "lldb/Core/Architecture.h"
+#include "lldb/Utility/ArchSpec.h"
+
+namespace lldb_private {
+
+class ArchitectureMips : public Architecture {
+public:
+  static ConstString GetPluginNameStatic();
+  static void Initialize();
+  static void Terminate();
+
+  ConstString GetPluginName() override;
+  uint32_t GetPluginVersion() override;
+
+  void OverrideStopInfo(Thread &thread) const override {}
+
+private:
+  lldb::addr_t DoGetBreakableLoadAddress(lldb::addr_t addr,
+                                       Target &) const override;
+
+  lldb::addr_t DoGetCallableLoadAddress(lldb::addr_t load_addr,
+                                        bool is_alternate_isa) const override;
+
+  lldb::addr_t DoGetOpcodeLoadAddress(lldb::addr_t load_addr) const override;
+
+  Instruction *GetInstructionAtAddress(const ExecutionContext &exe_ctx,
+                                       const Address &resolved_addr,
+                                       lldb::addr_t symbol_offset) const;
+
+
+  static std::unique_ptr<Architecture> Create(const ArchSpec &arch);
+  ArchitectureMips(const ArchSpec &arch) : m_arch(arch) {}
+
+  ArchSpec m_arch;
+};
+
+} // namespace lldb_private
+
+#endif // LLDB_PLUGIN_ARCHITECTURE_MIPS_H

source/Plugins/Architecture/Mips/ArchitectureMips.cpp

+//===-- ArchitectureMips.cpp -------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "Plugins/Architecture/Mips/ArchitectureMips.h"
+#include "lldb/Core/Address.h"
+#include "lldb/Core/Disassembler.h"
+#include "lldb/Core/Module.h"
+#include "lldb/Core/PluginManager.h"
+#include "lldb/Symbol/Function.h"
+#include "lldb/Symbol/SymbolContext.h"
+#include "lldb/Target/SectionLoadList.h"
+#include "lldb/Target/Target.h"
+#include "lldb/Utility/ArchSpec.h"
+#include "lldb/Utility/Log.h"
+
+using namespace lldb_private;
+using namespace lldb;
+
+ConstString ArchitectureMips::GetPluginNameStatic() {
+  return ConstString("mips");
+}
+
+void ArchitectureMips::Initialize() {
+  PluginManager::RegisterPlugin(GetPluginNameStatic(),
+                                "Mips-specific algorithms",
+                                &ArchitectureMips::Create);
+}
+
+void ArchitectureMips::Terminate() {
+  PluginManager::UnregisterPlugin(&ArchitectureMips::Create);
+}
+
+std::unique_ptr<Architecture> ArchitectureMips::Create(const ArchSpec &arch) {
+  return arch.IsMIPS() ?
+      std::unique_ptr<Architecture>(new ArchitectureMips(arch)) : nullptr;
+}
+
+ConstString ArchitectureMips::GetPluginName() { return GetPluginNameStatic(); }
+uint32_t ArchitectureMips::GetPluginVersion() { return 1; }
+
+addr_t ArchitectureMips::DoGetCallableLoadAddress(addr_t code_addr,
+                                                  bool is_alternate_isa) const {
+  if ((code_addr & 2ull) || is_alternate_isa)
+    return code_addr | 1u;
+  return code_addr;
+}
+
+addr_t ArchitectureMips::DoGetOpcodeLoadAddress(addr_t opcode_addr) const {
+  return opcode_addr & ~(1ull);
+}
+
+lldb::addr_t ArchitectureMips::DoGetBreakableLoadAddress(lldb::addr_t addr,
+                                                         Target &target) const {
+
+  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
+
+  Address resolved_addr;
+
+  SectionLoadList &section_load_list = target.GetSectionLoadList();
+  if (section_load_list.IsEmpty())
+    // No sections are loaded, so we must assume we are not running yet and
+    // need to operate only on file address.
+    target.ResolveFileAddress(addr, resolved_addr);
+  else
+    target.ResolveLoadAddress(addr, resolved_addr);
+
+  addr_t current_offset = 0;
+
+  // Get the function boundaries to make sure we don't scan back before the
+  // beginning of the current function.
+  ModuleSP temp_addr_module_sp(resolved_addr.GetModule());
+  if (temp_addr_module_sp) {
+    SymbolContext sc;
+    uint32_t resolve_scope = eSymbolContextFunction | eSymbolContextSymbol;
+    temp_addr_module_sp->ResolveSymbolContextForAddress(resolved_addr,
+      resolve_scope, sc);
+    Address sym_addr;
+    if (sc.function)
+      sym_addr = sc.function->GetAddressRange().GetBaseAddress();
+    else if (sc.symbol)
+      sym_addr = sc.symbol->GetAddress();
+
+    addr_t function_start = sym_addr.GetLoadAddress(&target);
+    if (function_start == LLDB_INVALID_ADDRESS)
+      function_start = sym_addr.GetFileAddress();
+
+    if (function_start)
+      current_offset = addr - function_start;
+  }
+
+  // If breakpoint address is start of function then we dont have to do
+  // anything.
+  if (current_offset == 0)
+    return addr;
+
+  ExecutionContext ctx;
+  target.CalculateExecutionContext(ctx);
+  auto insn = GetInstructionAtAddress(ctx, current_offset, addr);
+
+  if (nullptr == insn || !insn->HasDelaySlot())
+    return addr;
+
+  // Adjust the breakable address
+  auto breakable_addr = addr - insn->GetOpcode().GetByteSize();
+  if (log)
+    log->Printf("Target::%s Breakpoint at 0x%8.8" PRIx64
+      " is adjusted to 0x%8.8" PRIx64 " due to delay slot\n",
+      __FUNCTION__, addr, breakable_addr);
+
+  return breakable_addr;
+}
+
+Instruction *ArchitectureMips::GetInstructionAtAddress(
+    const ExecutionContext &exe_ctx, const Address &resolved_addr,
+    addr_t symbol_offset) const {
+
+  auto loop_count = symbol_offset / 2;
+
+  uint32_t arch_flags = m_arch.GetFlags();
+  bool IsMips16 = arch_flags & ArchSpec::eMIPSAse_mips16;
+  bool IsMicromips = arch_flags & ArchSpec::eMIPSAse_micromips;
+
+  if (loop_count > 3) {
+    // Scan previous 6 bytes
+    if (IsMips16 | IsMicromips)
+      loop_count = 3;
+    // For mips-only, instructions are always 4 bytes, so scan previous 4
+    // bytes only.
+    else
+      loop_count = 2;
+  }
+
+  // Create Disassembler Instance
+  lldb::DisassemblerSP disasm_sp(
+    Disassembler::FindPlugin(m_arch, nullptr, nullptr));
+
+  InstructionList instruction_list;
+  InstructionSP prev_insn;
+  bool prefer_file_cache = true; // Read from file
+  uint32_t inst_to_choose = 0;
+
+  Address addr = resolved_addr;
+
+  for (uint32_t i = 1; i <= loop_count; i++) {
+    // Adjust the address to read from.
+    addr.Slide(-2);
+    AddressRange range(addr, i * 2);
+    uint32_t insn_size = 0;
+
+    disasm_sp->ParseInstructions(&exe_ctx, range, nullptr, prefer_file_cache);
+
+    uint32_t num_insns = disasm_sp->GetInstructionList().GetSize();
+    if (num_insns) {
+      prev_insn = disasm_sp->GetInstructionList().GetInstructionAtIndex(0);
+      insn_size = prev_insn->GetOpcode().GetByteSize();
+      if (i == 1 && insn_size == 2) {
+        // This looks like a valid 2-byte instruction (but it could be a part
+        // of upper 4 byte instruction).
+        instruction_list.Append(prev_insn);
+        inst_to_choose = 1;
+      }
+      else if (i == 2) {
+        // Here we may get one 4-byte instruction or two 2-byte instructions.
+        if (num_insns == 2) {
+          // Looks like there are two 2-byte instructions above our
+          // breakpoint target address. Now the upper 2-byte instruction is
+          // either a valid 2-byte instruction or could be a part of it's
+          // upper 4-byte instruction. In both cases we don't care because in
+          // this case lower 2-byte instruction is definitely a valid
+          // instruction and whatever i=1 iteration has found out is true.
+          inst_to_choose = 1;
+          break;
+        }
+        else if (insn_size == 4) {
+          // This instruction claims its a valid 4-byte instruction. But it
+          // could be a part of it's upper 4-byte instruction. Lets try
+          // scanning upper 2 bytes to verify this.
+          instruction_list.Append(prev_insn);
+          inst_to_choose = 2;
+        }
+      }
+      else if (i == 3) {
+        if (insn_size == 4)
+          // FIXME: We reached here that means instruction at [target - 4] has
+          // already claimed to be a 4-byte instruction, and now instruction
+          // at [target - 6] is also claiming that it's a 4-byte instruction.
+          // This can not be true. In this case we can not decide the valid
+          // previous instruction so we let lldb set the breakpoint at the
+          // address given by user.
+          inst_to_choose = 0;
+        else
+          // This is straight-forward
+          inst_to_choose = 2;
+        break;
+      }
+    }
+    else {
+      // Decode failed, bytes do not form a valid instruction. So whatever
+      // previous iteration has found out is true.
+      if (i > 1) {
+        inst_to_choose = i - 1;
+        break;
+      }
+    }
+  }
+
+  // Check if we are able to find any valid instruction.
+  if (inst_to_choose) {
+    if (inst_to_choose > instruction_list.GetSize())
+      inst_to_choose--;
+    return instruction_list.GetInstructionAtIndex(inst_to_choose - 1).get();
+  }
+
+  return nullptr;
+}

source/Plugins/Architecture/Mips/CMakeLists.txt

+add_lldb_library(lldbPluginArchitectureMips PLUGIN
+  ArchitectureMips.cpp
+
+  LINK_LIBS
+    lldbCore
+    lldbTarget
+    lldbUtility
+  LINK_COMPONENTS
+    Support
+  )

source/Target/RegisterContext.cpp

 //===-- RegisterContext.cpp -------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
if (reg_info)
     return reg_info->name;
   return nullptr;
 }
 
 uint64_t RegisterContext::GetPC(uint64_t fail_value) {
   uint32_t reg = ConvertRegisterKindToRegisterNumber(eRegisterKindGeneric,
                                                      LLDB_REGNUM_GENERIC_PC);
   uint64_t pc = ReadRegisterAsUnsigned(reg, fail_value);
+  if (pc == fail_value)
+    return pc;
 
-  if (pc != fail_value) {
-    TargetSP target_sp = m_thread.CalculateTarget();
-    if (target_sp) {
-      Target *target = target_sp.get();
-      if (target)
-        pc = target->GetOpcodeLoadAddress(pc, AddressClass::eCode);
-    }
-  }
-
-  return pc;
+  return Architecture::GetOpcodeLoadAddress(target_sp.get(),
+                                            pc, AddressClass::eCode);
 }
 
 bool RegisterContext::SetPC(uint64_t pc) {
   uint32_t reg = ConvertRegisterKindToRegisterNumber(eRegisterKindGeneric,
                                                      LLDB_REGNUM_GENERIC_PC);
   bool success = WriteRegisterFromUnsigned(reg, pc);
   if (success) {
     StackFrameSP frame_sp(

source/Target/Target.cpp

 //===-- Target.cpp ----------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
BreakpointSP Target::CreateBreakpoint(const FileSpecList *containingModules,
   BreakpointResolverSP resolver_sp(new BreakpointResolverFileLine(
       nullptr, remapped_file, line_no, offset, check_inlines, skip_prologue,
       !static_cast<bool>(move_to_nearest_code)));
   return CreateBreakpoint(filter_sp, resolver_sp, internal, hardware, true);
 }
 
 BreakpointSP Target::CreateBreakpoint(lldb::addr_t addr, bool internal,
                                       bool hardware) {
-  Address so_addr;
-
   // Check for any reason we want to move this breakpoint to other address.
-  addr = GetBreakableLoadAddress(addr);
+  addr = Architecture::GetBreakableLoadAddress(this, addr);
 
   // Attempt to resolve our load address if possible, though it is ok if it
   // doesn't resolve to section/offset.
 
   // Try and resolve as a load address if possible
+  Address so_addr;
   GetSectionLoadList().ResolveLoadAddress(addr, so_addr);
   if (!so_addr.IsValid()) {
     // The address didn't resolve, so just set this as an absolute address
     so_addr.SetOffset(addr);
   }
   BreakpointSP bp_sp(CreateBreakpoint(so_addr, internal, hardware));
   return bp_sp;
 }
 
 BreakpointSP Target::CreateBreakpoint(const Address &addr, bool internal,
                                       bool hardware) {
   SearchFilterSP filter_sp(
       new SearchFilterForUnconstrainedSearches(shared_from_this()));
   BreakpointResolverSP resolver_sp(
m_scratch_type_system_map.ForEach(
           address = persistent_state->LookupSymbol(name);
           if (address != LLDB_INVALID_ADDRESS)
             return false; // Stop iterating the ForEach
         }
         return true; // Keep iterating the ForEach
       });
   return address;
 }
 
-lldb::addr_t Target::GetCallableLoadAddress(lldb::addr_t load_addr,
-                                            AddressClass addr_class) const {
-  addr_t code_addr = load_addr;
-  switch (m_arch.GetSpec().GetMachine()) {
-  case llvm::Triple::mips:
-  case llvm::Triple::mipsel:
-  case llvm::Triple::mips64:
-  case llvm::Triple::mips64el:
-    switch (addr_class) {
-    case AddressClass::eData:
-    case AddressClass::eDebug:
-      return LLDB_INVALID_ADDRESS;
-
-    case AddressClass::eUnknown:
-    case AddressClass::eInvalid:
-    case AddressClass::eCode:
-    case AddressClass::eCodeAlternateISA:
-    case AddressClass::eRuntime:
-      if ((code_addr & 2ull) || (addr_class == AddressClass::eCodeAlternateISA))
-        code_addr |= 1ull;
-      break;
-    }
-    break;
-
-  case llvm::Triple::arm:
-  case llvm::Triple::thumb:
-    switch (addr_class) {
-    case AddressClass::eData:
-    case AddressClass::eDebug:
-      return LLDB_INVALID_ADDRESS;
-
-    case AddressClass::eUnknown:
-    case AddressClass::eInvalid:
-    case AddressClass::eCode:
-    case AddressClass::eCodeAlternateISA:
-    case AddressClass::eRuntime:
-      // Check if bit zero it no set?
-      if ((code_addr & 1ull) == 0) {
-        // Bit zero isn't set, check if the address is a multiple of 2?
-        if (code_addr & 2ull) {
-          // The address is a multiple of 2 so it must be thumb, set bit zero
-          code_addr |= 1ull;
-        } else if (addr_class == AddressClass::eCodeAlternateISA) {
-          // We checked the address and the address claims to be the alternate
-          // ISA which means thumb, so set bit zero.
-          code_addr |= 1ull;
-        }
-      }
-      break;
-    }
-    break;
-
-  default:
-    break;
-  }
-  return code_addr;
-}
-
-lldb::addr_t Target::GetOpcodeLoadAddress(lldb::addr_t load_addr,
-                                          AddressClass addr_class) const {
-  addr_t opcode_addr = load_addr;
-  switch (m_arch.GetSpec().GetMachine()) {
-  case llvm::Triple::mips:
-  case llvm::Triple::mipsel:
-  case llvm::Triple::mips64:
-  case llvm::Triple::mips64el:
-  case llvm::Triple::arm:
-  case llvm::Triple::thumb:
-    switch (addr_class) {
-    case AddressClass::eData:
-    case AddressClass::eDebug:
-      return LLDB_INVALID_ADDRESS;
-
-    case AddressClass::eInvalid:
-    case AddressClass::eUnknown:
-    case AddressClass::eCode:
-    case AddressClass::eCodeAlternateISA:
-    case AddressClass::eRuntime:
-      opcode_addr &= ~(1ull);
-      break;
-    }
-    break;
-
-  default:
-    break;
-  }
-  return opcode_addr;
-}
-
-lldb::addr_t Target::GetBreakableLoadAddress(lldb::addr_t addr) {
-  addr_t breakable_addr = addr;
-  Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
-
-  switch (m_arch.GetSpec().GetMachine()) {
-  default:
-    break;
-  case llvm::Triple::mips:
-  case llvm::Triple::mipsel:
-  case llvm::Triple::mips64:
-  case llvm::Triple::mips64el: {
-    addr_t function_start = 0;
-    addr_t current_offset = 0;
-    uint32_t loop_count = 0;
-    Address resolved_addr;
-    uint32_t arch_flags = m_arch.GetSpec().GetFlags();
-    bool IsMips16 = arch_flags & ArchSpec::eMIPSAse_mips16;
-    bool IsMicromips = arch_flags & ArchSpec::eMIPSAse_micromips;
-    SectionLoadList &section_load_list = GetSectionLoadList();
-
-    if (section_load_list.IsEmpty())
-      // No sections are loaded, so we must assume we are not running yet and
-      // need to operate only on file address.
-      m_images.ResolveFileAddress(addr, resolved_addr);
-    else
-      section_load_list.ResolveLoadAddress(addr, resolved_addr);
-
-    // Get the function boundaries to make sure we don't scan back before the
-    // beginning of the current function.
-    ModuleSP temp_addr_module_sp(resolved_addr.GetModule());
-    if (temp_addr_module_sp) {
-      SymbolContext sc;
-      uint32_t resolve_scope = eSymbolContextFunction | eSymbolContextSymbol;
-      temp_addr_module_sp->ResolveSymbolContextForAddress(resolved_addr,
-                                                          resolve_scope, sc);
-      Address sym_addr;
-      if (sc.function)
-        sym_addr = sc.function->GetAddressRange().GetBaseAddress();
-      else if (sc.symbol)
-        sym_addr = sc.symbol->GetAddress();
-
-      function_start = sym_addr.GetLoadAddress(this);
-      if (function_start == LLDB_INVALID_ADDRESS)
-        function_start = sym_addr.GetFileAddress();
-
-      if (function_start)
-        current_offset = addr - function_start;
-    }
-
-    // If breakpoint address is start of function then we dont have to do
-    // anything.
-    if (current_offset == 0)
-      return breakable_addr;
-    else
-      loop_count = current_offset / 2;
-
-    if (loop_count > 3) {
-      // Scan previous 6 bytes
-      if (IsMips16 | IsMicromips)
-        loop_count = 3;
-      // For mips-only, instructions are always 4 bytes, so scan previous 4
-      // bytes only.
-      else
-        loop_count = 2;
-    }
-
-    // Create Disassembler Instance
-    lldb::DisassemblerSP disasm_sp(
-        Disassembler::FindPlugin(m_arch.GetSpec(), nullptr, nullptr));
-
-    ExecutionContext exe_ctx;
-    CalculateExecutionContext(exe_ctx);
-    InstructionList instruction_list;
-    InstructionSP prev_insn;
-    bool prefer_file_cache = true; // Read from file
-    uint32_t inst_to_choose = 0;
-
-    for (uint32_t i = 1; i <= loop_count; i++) {
-      // Adjust the address to read from.
-      resolved_addr.Slide(-2);
-      AddressRange range(resolved_addr, i * 2);
-      uint32_t insn_size = 0;
-
-      disasm_sp->ParseInstructions(&exe_ctx, range, nullptr, prefer_file_cache);
-
-      uint32_t num_insns = disasm_sp->GetInstructionList().GetSize();
-      if (num_insns) {
-        prev_insn = disasm_sp->GetInstructionList().GetInstructionAtIndex(0);
-        insn_size = prev_insn->GetOpcode().GetByteSize();
-        if (i == 1 && insn_size == 2) {
-          // This looks like a valid 2-byte instruction (but it could be a part
-          // of upper 4 byte instruction).
-          instruction_list.Append(prev_insn);
-          inst_to_choose = 1;
-        } else if (i == 2) {
-          // Here we may get one 4-byte instruction or two 2-byte instructions.
-          if (num_insns == 2) {
-            // Looks like there are two 2-byte instructions above our
-            // breakpoint target address. Now the upper 2-byte instruction is
-            // either a valid 2-byte instruction or could be a part of it's
-            // upper 4-byte instruction. In both cases we don't care because in
-            // this case lower 2-byte instruction is definitely a valid
-            // instruction and whatever i=1 iteration has found out is true.
-            inst_to_choose = 1;
-            break;
-          } else if (insn_size == 4) {
-            // This instruction claims its a valid 4-byte instruction. But it
-            // could be a part of it's upper 4-byte instruction. Lets try
-            // scanning upper 2 bytes to verify this.
-            instruction_list.Append(prev_insn);
-            inst_to_choose = 2;
-          }
-        } else if (i == 3) {
-          if (insn_size == 4)
-            // FIXME: We reached here that means instruction at [target - 4] has
-            // already claimed to be a 4-byte instruction, and now instruction
-            // at [target - 6] is also claiming that it's a 4-byte instruction.
-            // This can not be true. In this case we can not decide the valid
-            // previous instruction so we let lldb set the breakpoint at the
-            // address given by user.
-            inst_to_choose = 0;
-          else
-            // This is straight-forward
-            inst_to_choose = 2;
-          break;
-        }
-      } else {
-        // Decode failed, bytes do not form a valid instruction. So whatever
-        // previous iteration has found out is true.
-        if (i > 1) {
-          inst_to_choose = i - 1;
-          break;
-        }
-      }
-    }
-
-    // Check if we are able to find any valid instruction.
-    if (inst_to_choose) {
-      if (inst_to_choose > instruction_list.GetSize())
-        inst_to_choose--;
-      prev_insn = instruction_list.GetInstructionAtIndex(inst_to_choose - 1);
-
-      if (prev_insn->HasDelaySlot()) {
-        uint32_t shift_size = prev_insn->GetOpcode().GetByteSize();
-        // Adjust the breakable address
-        breakable_addr = addr - shift_size;
-        if (log)
-          log->Printf("Target::%s Breakpoint at 0x%8.8" PRIx64
-                      " is adjusted to 0x%8.8" PRIx64 " due to delay slot\n",
-                      __FUNCTION__, addr, breakable_addr);
-      }
-    }
-    break;
-  }
-  }
-  return breakable_addr;
-}
-
 SourceManager &Target::GetSourceManager() {
   if (!m_source_manager_ap)
     m_source_manager_ap.reset(new SourceManager(shared_from_this()));
   return *m_source_manager_ap;
 }
 
 ClangModulesDeclVendor *Target::GetClangModulesDeclVendor() {
   static std::mutex s_clang_modules_decl_vendor_mutex; // If this is contended
                                                        // we can make it
                                                        // per-target
 

source/Target/ThreadPlanRunToAddress.cpp

 //===-- ThreadPlanRunToAddress.cpp ------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // C Includes

 }
 
 ThreadPlanRunToAddress::ThreadPlanRunToAddress(Thread &thread,
                                                lldb::addr_t address,
                                                bool stop_others)
     : ThreadPlan(ThreadPlan::eKindRunToAddress, "Run to address plan", thread,
                  eVoteNoOpinion, eVoteNoOpinion),
       m_stop_others(stop_others), m_addresses(), m_break_ids() {
-  m_addresses.push_back(
-      m_thread.CalculateTarget()->GetOpcodeLoadAddress(address));
+  Target &target = thread.GetProcess()->GetTarget();
+  m_addresses.push_back(Architecture::GetOpcodeLoadAddress(&target, address));
   SetInitialBreakpoints();
 }
 
 ThreadPlanRunToAddress::ThreadPlanRunToAddress(
     Thread &thread, const std::vector<lldb::addr_t> &addresses,
     bool stop_others)
     : ThreadPlan(ThreadPlan::eKindRunToAddress, "Run to address plan", thread,
                  eVoteNoOpinion, eVoteNoOpinion),
       m_stop_others(stop_others), m_addresses(addresses), m_break_ids() {
   // Convert all addresses into opcode addresses to make sure we set
   // breakpoints at the correct address.
   Target &target = thread.GetProcess()->GetTarget();
-  std::vector<lldb::addr_t>::iterator pos, end = m_addresses.end();
-  for (pos = m_addresses.begin(); pos != end; ++pos)
-    *pos = target.GetOpcodeLoadAddress(*pos);
+  for (auto &addr : m_addresses)
+    addr = Architecture::GetOpcodeLoadAddress(&target, addr);
 
   SetInitialBreakpoints();
 }
 
 void ThreadPlanRunToAddress::SetInitialBreakpoints() {
   size_t num_addresses = m_addresses.size();
   m_break_ids.resize(num_addresses);