clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed.
It would be nice to see context when you submit diffs. with SVN: svn diff -x -U9999999 ... Or git: git diff -U9999999 ================ Comment at: include/lldb/Core/Architecture.h:15 +class DynamicRegisterInfo; + ---------------- Is DynamicRegisterInfo really in the top level namespace? ================ Comment at: include/lldb/Core/Architecture.h:18 namespace lldb_private { +class ArchSpec; ---------------- include the lldb-forward.h and remove this ================ Comment at: include/lldb/Utility/ArchSpec.h:91-92 + // ARC configuration flags + enum ARCflags { eARC_rf32 = 0 /*0b00*/, eARC_rf16 = 2 /*0b10*/ }; + ---------------- It would be great to find a way that ArchSpec.h doesn't get polluted with everyone's flags by putting the flag definitions in the Architecture.h file. Not mandatory for this patch, but something to think about. ================ Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:76-93 +void ArchitectureArc::AdjustRegisterInfo(DynamicRegisterInfo ®_info) const { + auto reg_count = reg_info.GetNumRegisters(); + decltype(reg_count) dwarf_index = 0; + for (decltype(reg_count) i = 0; i < reg_count; ++i) { + auto ® = *reg_info.GetRegisterInfoAtIndex(i); + auto ®_num = reg.kinds[eRegisterKindDWARF]; + if (LLDB_INVALID_REGNUM == reg_num) { ---------------- What is this code doing? Please add comments. Also, you have a ConfigureArchitecture in ProcessGDBRemote.cpp, can't you just do this work in that function and avoid the need to add Architecture::AdjustRegisterInfo() in the first place? And if so, is the architecture plug-in even needed? ================ Comment at: source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:625 + case llvm::Triple::arc: + { ---------------- Context would really help here to see the surrounding code. ================ Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4530-4541 + const RegisterInfo *rf_build_info = nullptr; + + const auto reg_count = dyn_reg_info.GetNumRegisters(); + // At least 16 first registers are not BCRs. + decltype(reg_count) skip_gpr = 16u; + for (auto reg_num = skip_gpr; reg_num < reg_count; ++reg_num) { + const auto reg_info = dyn_reg_info.GetRegisterInfoAtIndex(reg_num); ---------------- Make: ``` const lldb_private::RegisterInfo *DynamicRegisterInfo::GetRegisterInfo(const lldb_private::ConstString ®_name) const; ``` public and call it. No need to duplicate the name search here.. Code will be: ``` const RegisterInfo *rf_build_info = dyn_reg_info.GetRegisterInfo(reg_name); ``` ================ Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4683-4691 + if (llvm::Triple::arc == arch_to_use.GetMachine()) { + if (!ConfigureArchitecture(*this, m_register_info, arch_to_use)) + return false; + GetTarget().SetArchitecture(arch_to_use); } + + auto arch_plugin = GetTarget().GetArchitecturePlugin(); ---------------- This code seems like it could be cleaned up. - ConfigureArchitecture should be named ConfigureARCArchitecture if it truly is ARC specfic - why doesn't ConfigureArchitecture call GetTarget().SetArchitecture(arch_to_use) itself? - Do we really want to return if ConfigureArchitecture() returns false? Do we not want to adjust the register info? - why doesn't ConfigureArchitecture just fix up the dynamic register info and avoid the call to AdjustRegisterInfo in an architecture plug-in ================ Comment at: source/Target/Platform.cpp:1872-1876 + case llvm::Triple::arc: { + static const uint8_t g_hex_opcode[] = { 0xff, 0x7f }; + trap_opcode = g_hex_opcode; + trap_opcode_size = sizeof(g_hex_opcode); + } break; ---------------- The software breakpoint opcode would be a great candidate to move to the Architecture plug-ins. Not needed for this patch since this is how is has been done for all architectures up till now, but just noting this here. ================ Comment at: source/Target/Target.cpp:70 : m_spec(spec), - m_plugin_up(PluginManager::CreateArchitectureInstance(spec)) {} + m_plugin_up(PluginManager::CreateArchitectureInstance(m_spec)) {} ---------------- is this change needed? ================ Comment at: source/Target/Target.cpp:74 m_spec = spec; - m_plugin_up = PluginManager::CreateArchitectureInstance(spec); + m_plugin_up = PluginManager::CreateArchitectureInstance(m_spec); return *this; ---------------- is this change needed? Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55718/new/ https://reviews.llvm.org/D55718 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits