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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
