[Lldb-commits] [PATCH] D48623: Move architecture-specific address adjustment to architecture plugins.

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

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



================
Comment at: include/lldb/Target/Target.h:750
                                    BreakpointIDList &new_bps);
 
   void ModulesDidLoad(ModuleList &module_list);
----------------
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.


================
Comment at: source/Core/Address.cpp:333
 
-  if (code_addr == LLDB_INVALID_ADDRESS)
+  if (code_addr == LLDB_INVALID_ADDRESS || !target)
     return code_addr;
----------------
None of the changes in Address.cpp are needed if we leave the functions in Target.


================
Comment at: source/Target/RegisterContext.cpp:132-140
+  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);
-    }
-  }
+  TargetSP target_sp = m_thread.CalculateTarget();
+  if (!target_sp)
+    return pc;
 
----------------
None of these changes are needed if we leave the function in Target.h/.cpp


================
Comment at: source/Target/Target.cpp:380-382
+  auto arch_plugin = GetArchitecturePlugin();
+  if (arch_plugin)
+    addr = arch_plugin->GetBreakableLoadAddress(addr, *this);
----------------
None of these changes are needed if we leave the function in Target.h/.cpp




================
Comment at: source/Target/Target.cpp:2429-2430
 
-lldb::addr_t Target::GetCallableLoadAddress(lldb::addr_t load_addr,
-                                            AddressClass addr_class) const {
-  addr_t code_addr = load_addr;
----------------
Leave this function in and call through to arch plug-in if we have one.


================
Comment at: source/Target/Target.cpp:2487-2488
-
-lldb::addr_t Target::GetOpcodeLoadAddress(lldb::addr_t load_addr,
-                                          AddressClass addr_class) const {
-  addr_t opcode_addr = load_addr;
----------------
Leave this function in and call through to arch plug-in if we have one.


================
Comment at: source/Target/Target.cpp:2518
-
-lldb::addr_t Target::GetBreakableLoadAddress(lldb::addr_t addr) {
-  addr_t breakable_addr = addr;
----------------
Leave this function in and call through to arch plug-in if we have one.


================
Comment at: source/Target/ThreadPlanRunToAddress.cpp:45-47
+  auto arch_plugin = thread.GetProcess()->GetTarget().GetArchitecturePlugin();
   m_addresses.push_back(
+    arch_plugin ? arch_plugin->GetOpcodeLoadAddress(address) : address);
----------------
None of these changes are needed if we leave the function in Target.h/.cpp




================
Comment at: source/Target/ThreadPlanRunToAddress.cpp:59-63
+  auto arch_plugin = thread.GetProcess()->GetTarget().GetArchitecturePlugin();
+  if (arch_plugin) {
+    for (auto &addr : m_addresses)
+      addr = arch_plugin->GetOpcodeLoadAddress(addr);
+  }
----------------
None of these changes are needed if we leave the function in Target.h/.cpp




Repository:
  rLLDB LLDB

https://reviews.llvm.org/D48623