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