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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
