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

Wed Sep 21 07:50:32 PDT 2022

labath added a comment.

In D134041#3805034 <https://reviews.llvm.org/D134041#3805034>, @DavidSpickett wrote:

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

I think that part of the problem is that nobody has a full picture of how RegisterInfos work anymore. :)

I don't claim to have this fully thought out, but the idea goes roughly like this. For the past few years, we've been moving towards a world where LLDB is able to fill in lots of details about the target registers. I think we're now at a state where it is sufficient for the remote stub to specify the register name and number, and lldb will be able to fill on most of the details about that register: EH/DWARF/"generic" numbers, subregisters, etc. However, this code is only invoked when communicating remote stub -- not for core files.
On one hand, this kind of makes sense -- for core files, we are the source of the register info, so why wouldn't we provide the complete info straight away? However, it means that the information that "`ah` is a one byte sub-register of `rax` at offset 1" is repeated multiple times (we currently have three core file plugins, times the number of architectures they support). If we made core file register infos go through this "augmentation" process, then we could unify our core file/live process flow more, and relieve the core file plugins of the burden of dealing with the subregisters -- all they'd need to know is how to read/write whole registers, and the generic code would take care of all the subregisters.
This would also mean that *all* register infos being handled by generic code would be DynamicRegisterInfos, which means we could drop the avoid this POD business, and just replace that class with something modern and easy to use. The only place where we would need to store static arrays would be in the bowels of individual plugins, but these would be simpler than the current RegisterInfo struct, as a lot of this info would be deduced (maybe including the register type information that you're trying to introduce), and we might even have each plugin store the info in whichever format it sees fit -- the only requirement would be that a DynamicRegisterInfo comes out at the end. Some plugins may choose not to store static info at all, as we're already running into the limits of what can be stored statically -- if an architecture has multiple optional registers sets (whose presence is only known at runtime), then its impossible to stuff those registers into a static array -- I believe all our AArch64 registers are currently dynamic for this reason.

I know this is somewhat vague, but that's why this is just an idea. Someone would have to try it out to find all the issues and figure them out. I can try to help if you want to take it on.

================
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
----------------
DavidSpickett wrote:
> labath wrote:
> > 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.
> True, but sounds like we're going toward adding a pointer to the info. So that should keep the size constant.
It should, but regardless of that, I'm surprised to see the structs being stored here, I'm not aware of any other place which stores RegisterInfos be value.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134041