[lldb] Introduce DynamicRegisterInfo::CreateFromDict

[bulbazord]

Summary

I want to add some error handling to DynamicRegisterInfo because there
are many operations that can fail and many of these operations do not
give meaningful information back to the caller.

To begin that process, I want to add a static method that is responsible
for creating a DynamicRegisterInfo from a StructuredData::Dictionary
(and ArchSpec). This is meant to replace the equivalent constructor
because constructors are ill-equipped to perform error handling.

[mib]

Are we replacing all the constructors that can fail by static methods ? Wouldn't it be easier to pass an Status& as an extra parameter ?

[bulbazord]

In D152594#4410620, @mib wrote:

Are we replacing all the constructors that can fail by static methods ? Wouldn't it be easier to pass an Status& as an extra parameter ?

It would probably be easier to do, but I don't think it actually solves the problems that I'm trying to solve.

1. Status as a construct is too easy to ignore. There are plenty of examples of methods that take Status & where the callers create a Status object only to drop it on the floor afterwards. There is no consequence for ignoring a Status after calling a method that can fail and I don't want this to be another instance where people can just ignore error handling if it's inconvenient. I understand that it's possible to ignore the results of llvm::Error and llvm::Expected, but this requires deliberate intent and is a lot easier to grep for. Right now, this returns a std::unique_ptr but perhaps it can return some kind of llvm::Error or llvm::Expected in the future.
2. Fallible constructors are difficult to get right from an error-handling perspective. Even if we pass a Status & and you can enforce that all callers actually check it, the constructor still gives you back a DynamicRegisterInfo object. For example, take the code:

Status error;
m_dyn_reg_info_sp = std::make_shared<DynamicRegisterInfo>(error);
if (error.Fail())
  return error;

In this example, even if you take the error handling path, m_dyn_reg_info_sp (of type std::shared_ptr<DynamicRegisterInfo>) is still set to point to an actual DynamicRegisterInfo object. Later on you might try to do something like:

if (m_dyn_reg_info_sp) {
  // use m_dyn_reg_info_sp
}

This block will execute, possibly leading to strange or difficult to debug issues. A static factory method means that if initialization fails, we do not get back an object.

[bulbazord]

Renamed from CreateFromDict to Create to be more consistent with the rest of LLDB

[JDevlieghere]

LGTM

lldb/include/lldb/Target/DynamicRegisterInfo.h

struct Register {
     std::vector<uint32_t> invalidate_regs;
     uint32_t value_reg_offset = 0;
     // Non-null if there is an XML provided type.
     const RegisterFlags *flags_type = nullptr;
   };
 
   DynamicRegisterInfo() = default;
 
-  DynamicRegisterInfo(const lldb_private::StructuredData::Dictionary &dict,
-                      const lldb_private::ArchSpec &arch);
+  static std::unique_ptr<DynamicRegisterInfo>
+  Create(const StructuredData::Dictionary &dict, const ArchSpec &arch);
 
   virtual ~DynamicRegisterInfo() = default;
 
   DynamicRegisterInfo(DynamicRegisterInfo &&info);
   DynamicRegisterInfo &operator=(DynamicRegisterInfo &&info);
 
   size_t SetRegisterInfo(const lldb_private::StructuredData::Dictionary &dict,
                          const lldb_private::ArchSpec &arch);

lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp

LLDB_LOGF(log,
               "thread register definitions from python for pid %" PRIu64,
               m_process->GetID());
 
     StructuredData::DictionarySP dictionary =
         m_interpreter->OSPlugin_RegisterInfo(m_python_object_sp);
     if (!dictionary)
       return nullptr;
 
-    m_register_info_up = std::make_unique<DynamicRegisterInfo>(
+    m_register_info_up = DynamicRegisterInfo::Create(
         *dictionary, m_process->GetTarget().GetArchitecture());
+    assert(m_register_info_up);
     assert(m_register_info_up->GetNumRegisters() > 0);
     assert(m_register_info_up->GetNumRegisterSets() > 0);
   }
   return m_register_info_up.get();
 }
 
 bool OperatingSystemPython::UpdateThreadList(ThreadList &old_thread_list,
                                              ThreadList &core_thread_list,

lldb/source/Plugins/Process/scripted/ScriptedThread.cpp

if (!m_register_info_sp) {
 
     Status error;
     if (!reg_info)
       return ScriptedInterface::ErrorWithMessage<
           std::shared_ptr<DynamicRegisterInfo>>(
           LLVM_PRETTY_FUNCTION, "Failed to get scripted thread registers info.",
           error, LLDBLog::Thread);
 
-    m_register_info_sp = std::make_shared<DynamicRegisterInfo>(
+    m_register_info_sp = DynamicRegisterInfo::Create(
         *reg_info, m_scripted_process.GetTarget().GetArchitecture());
   }
 
   return m_register_info_sp;
 }
 
 StructuredData::ObjectSP ScriptedThread::FetchThreadExtendedInfo() {
   CheckInterpreterAndScriptObject();

lldb/source/Target/DynamicRegisterInfo.cpp

 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/StringExtractor.h"
 #include "lldb/Utility/StructuredData.h"
 
 using namespace lldb;
 using namespace lldb_private;
 
-DynamicRegisterInfo::DynamicRegisterInfo(
-    const lldb_private::StructuredData::Dictionary &dict,
-    const lldb_private::ArchSpec &arch) {
-  SetRegisterInfo(dict, arch);
+std::unique_ptr<DynamicRegisterInfo>
+DynamicRegisterInfo::Create(const StructuredData::Dictionary &dict,
+                            const ArchSpec &arch) {
+  auto dyn_reg_info = std::make_unique<DynamicRegisterInfo>();
+  if (!dyn_reg_info)
+    return nullptr;
+
+  if (dyn_reg_info->SetRegisterInfo(dict, arch) == 0)
+    return nullptr;
+
+  return dyn_reg_info;
 }
 
 DynamicRegisterInfo::DynamicRegisterInfo(DynamicRegisterInfo &&info) {
   MoveFrom(std::move(info));
 }
 
 DynamicRegisterInfo &
 DynamicRegisterInfo::operator=(DynamicRegisterInfo &&info) {