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

[David Spickett via Phabricator via lldb-commits]

DavidSpickett 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).

Agreed. I think I stumbled into a solution for this for different reasons.

I might have mentioned adding a std::string somewhere in this review but I only did that as a way to test this patch actually would work.

> 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).

Agreed.

> 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.

Sure I'd be interested in that. I've just been hacking on this small part of it so I don't have the full picture yet.

> So I might recommend adding a RegisterInfo member that is a "Fields *fields" that can be easily set to NULL in POD initializers, but then stored in the dynamic register info class in its own vector and then have the RegisterInfo struct point to another POD definition of the Fields.

I though along the same lines but a different reason, I didn't want to store empty <register flags type> in every register info that didn't have flags and balloon the total size.

So I have:

  struct RegisterInfo {
  <...>
    /// If not nullptr, a custom type defined by XML descriptions.
    /// Using shared_ptr here means that the majority of registers that don't have
    /// custom types do not have to waste sizeof(RegisterFlags) bytes.
    std::shared_ptr<RegisterFlags> flags_type = nullptr;

But shared_ptr still has a destructor. It could be `RegisterFlags*`, I just have to work out when all that data needs to be dealocated. Probably when the target itself destructs but I need to figure that out, in general i don't know yet where the best place for these flags/fields to live is.

If it is a plain pointer then this patch is redundant but hey I learned how to use placement new :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134041