clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed.
================ Comment at: include/lldb/Core/Architecture.h:118-127 + static constexpr uint8_t max_features_count = 32u; + virtual std::bitset<max_features_count> GetFeatures() const { return 0; } + + using ReadRegister = std::function<llvm::Optional<RegisterValue>( + ConstString /*name*/, bool /*case_sensitive*/)>; + virtual bool SetFeatures(const ReadRegister &func) { return true; } + ---------------- We currently have been using the lldb_private::Flags class for this kind of stuff. Best to use that here. ================ Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:29 + PluginManager::RegisterPlugin(GetPluginNameStatic(), + "ARC-specific algorithms", + &ArchitectureArc::Create); ---------------- Not a great human readable architecture name here. All other plug-ins use the short architecture name ("arm", "mipc", "ppc64"). Best to just use "arc"? ================ Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.cpp:46-78 +bool ArchitectureArc::SetFeatures(const ReadRegister &func) { + + auto set_feature = [&func, this]( + ConstString reg_name, uint_least32_t field_mask, size_t feature_position) + -> llvm::Optional<uint64_t> { + const bool case_sensitive = false; + auto opt_value = func(reg_name, case_sensitive); ---------------- All this code belongs elsewhere. SetFeatures probably needs to be changed to be: Architecture::GetFlags().XXX() where XXX is a method on the lldb_private::Flags class, so this code should go where the code that was calling it was. Kind of weird to have register reading function passed in. ================ Comment at: source/Plugins/Architecture/Arc/ArchitectureArc.h:32-40 + std::bitset<max_features_count> GetFeatures() const override { + return m_features; + } + + bool SetFeatures(const ReadRegister &func) override; + + bool MatchFeatures(const ArchSpec &spec) const override; ---------------- Use lldb_private::Flags ================ Comment at: source/Plugins/Process/Utility/DynamicRegisterInfo.cpp:642-669 + switch (reg.kinds[eRegisterKindDWARF]) { + case 0: + reg.kinds[eRegisterKindGeneric] = LLDB_REGNUM_GENERIC_ARG1; + break; + case 1: + reg.kinds[eRegisterKindGeneric] = LLDB_REGNUM_GENERIC_ARG2; + break; ---------------- Magic numbers? Can we use enums? ================ Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:1222-1223 + + if (!arch_plugin->SetFeatures(reg_reader) && log) + log->PutCString("Failed to set architecture features"); + ---------------- All this work should be done in this function then use arch_plug->GetFlags().Set(mask); 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