[Lldb-commits] [PATCH] D134041: [LLDB] Enable non-trivial types in EmulateInstruction::Context

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I think the main reason that the RegisterInfo struct is such as it is, is because it wants to be trivially constructible (or whatever the c++ term for that is). I'm pretty sure that adding a std::string member will make it stop being that, and I don't think we suddenly want to run global constructors for all of the global RegisterInfo instances we have (and we have a lot of them).

If you wanted to preserve the existing patterns, the right way to store a string/array member inside a static struct would be to copy what is being done in the value_regs/invalidate_regs field, i.e., store the value itself in another static object, and then store the pointer to that object into RegisterInfo. For dynamic RegisterInfos (like those received from the lldb-server), you'd use DynamicRegisterInfo and DynamicRegisterInfo::Register, which have logic to persist/create storage for these values (that logic would have to be extended to handle the new fields as well).

That said I would *love* is someone changed the RegisterInfo structs into something saner, but I think that will need to be more elaborate than simply stuffing a std::vector member into it. I can give you my idea of how this could work, if you're interested in trying this out.



================
Comment at: lldb/include/lldb/Core/EmulateInstruction.h:196
+      struct RegisterPlusOffsetStruct {
         RegisterInfo reg;      // base register
         int64_t signed_offset; // signed offset added to base register
----------------
I actually think that the simplest solution here would be to store the RegisterInfos as pointers. Copying them around doesn't make sense, particularly if their size is about to grow.


================
Comment at: lldb/include/lldb/Core/EmulateInstruction.h:342-345
+      info.~ContextUnion();
+      info.info_type = eInfoTypeRegisterRegisterOperands;
+      new (&info.RegisterRegisterOperands.operand1) RegisterInfo(op1_reg);
+      new (&info.RegisterRegisterOperands.operand2) RegisterInfo(op2_reg);
----------------
I am not a C++ expert, but I have a strong suspicion that this is not correct. You're destroyed the whole object, but then are only recreating its individual submembers. I get that these are the only members which have non-trivial constructors, but I don't think that's how c++ works. I can confirm this with some c++ experts if you want, but I am pretty sure that you need to recreate the ContextUnion object as a whole here to avoid straying into UB territory.


================
Comment at: lldb/include/lldb/lldb-private-types.h:67-77
+  RegisterInfo() { kinds.fill(LLDB_INVALID_REGNUM); }
+
+  RegisterInfo(const char *name, const char *alt_name, uint32_t byte_size,
+               uint32_t byte_offset, lldb::Encoding encoding,
+               lldb::Format format,
+               std::array<uint32_t, lldb::kNumRegisterKinds> kinds,
+               uint32_t *value_regs, uint32_t *invalidate_regs)
----------------
Is this class still trivially constructible (not requiring dynamic initialization)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134041/new/

https://reviews.llvm.org/D134041