[Lldb-commits] [PATCH] D111136: [lldb] [DynamicRegisterInfo] Support iterating over registers()

[Michał Górny via Phabricator via lldb-commits]

mgorny added inline comments.


================
Comment at: lldb/include/lldb/Target/DynamicRegisterInfo.h:82
+  llvm::iterator_range<reg_collection::iterator> Registers() {
+    return llvm::iterator_range<reg_collection::iterator>(m_regs);
+  }
----------------
mgorny wrote:
> labath wrote:
> > mgorny wrote:
> > > ted wrote:
> > > > mgorny wrote:
> > > > > labath wrote:
> > > > > > Could this return const iterators? It seems we already have some non-const accessors, but I'd rather not propagate that..
> > > > > It can't — we're using these iterators to augment register infos ;-).
> > > > Maybe make a non-const protected, and a const public, so random plugin #37 can't modify register info?
> > > That would work for me; however, we'd have to make `ABI` and `DynamicRegisterInfo` `friend`s then. @labath, what do you thik?
> > Yeah, I forgot what motivated this change...
> > 
> > I don't think friending would work the way you imagined it (one would need some extra tricks) as friendship is not inherited by subclasses. That wouldn't be the end of the world, but I am hoping we can come with a better solution.
> > 
> > So, currently we have this `RemoteRegisterInfo` struct as a kind of a DynamicRegisterInfo precursor. What if the ABI classes operated on that instead ? Besides solving the visibility issues (this data isn't live yet, so we're free to modify it as we see fit), I'm hoping it would also make things simpler as the data hasn't been converted to the C format yet. We could move RemoteRegisterInfo to the DynamicRegisterInfo class (and rename it to something else, obviously) so that it's visible to everyone involved.
> > 
> > Currently Finalization happens very close to the point where we convert the remote format, so I'm hoping this wouldn't be too hard to implement. The presence of the `HardcodeARMRegisters` calls complicates things a bit. The main difference would be that these hardcoded registers wouldn't go through the augmentation process (without additional refactoring), but from the looks of it, they don't actually need any augmentation, so we could just let them float.
> > 
> > WDYT?
> Sure, I'll see if this can be done cleanly.
So I've tried, and… well, it's technically doable but it feels like I'm reinventing the wheel every step of the way. I have to manually iterate over all registers to find the base registers. I have to manually iterate over the registers to check if supplementary registers exist or not. And in the end, I have to somehow figure out which registers to pass via `AddRegister()` and which via `AddSupplementaryRegister()`.

At the same time, it really feels like `DynamicRegisterInfo` exists precisely to handle this kind of work, so avoiding it ends up feeling backwards.

Maybe we should instead look into making `DynamicRegisterInfo` immutable after finalization — either via making sure callers get only `const DynamicRegisterInfo&`s or even converting it into an `ImmutableDynamicRegisterInfo` of some kind.


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

https://reviews.llvm.org/D111136