Page MenuHomePhabricator

[lldb] [DynamicRegisterInfo] Support iterating over registers()

Authored by mgorny on Oct 5 2021, 5:19 AM.



Add DynamicRegisterInfo::registers() method that returns
llvm::iterator_range<> over RegisterInfos. This is a convenient
replacement for GetNumRegisters() + GetRegisterInfoAtIndex().

Diff Detail

Event Timeline

mgorny requested review of this revision.Oct 5 2021, 5:19 AM
mgorny created this revision.
labath added inline comments.Oct 5 2021, 5:31 AM

llvm (and, surprisingly, lldb -- I guess because its a new feature) follows c++ naming conventions for methods like these.


Could this return const iterators? It seems we already have some non-const accessors, but I'd rather not propagate that..

mgorny added inline comments.Oct 5 2021, 6:45 AM

/me not understand.


It can't — we're using these iterators to augment register infos ;-).

ted added a subscriber: ted.Oct 5 2021, 7:42 AM
ted added inline comments.

Maybe make a non-const protected, and a const public, so random plugin #37 can't modify register info?

mgorny added inline comments.Oct 5 2021, 8:19 AM

That would work for me; however, we'd have to make ABI and DynamicRegisterInfo friends then. @labath, what do you thik?

labath added inline comments.Oct 6 2021, 4:15 AM

iterator_range-returning functions use the C++ snake_case naming convention for the most part: implicit_operands, info_section_units, external_symbols, ...


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.


mgorny added inline comments.Oct 6 2021, 5:09 AM

Sure, I'll see if this can be done cleanly.

mgorny updated this revision to Diff 377516.Oct 6 2021, 6:02 AM

Switch to a const_iterator.

mgorny updated this revision to Diff 377520.Oct 6 2021, 6:06 AM
mgorny retitled this revision from [lldb] [DynamicRegisterInfo] Support iterating over Registers() to [lldb] [DynamicRegisterInfo] Support iterating over registers().
mgorny edited the summary of this revision. (Show Details)

Switch to lowercase name.

labath accepted this revision.Oct 6 2021, 7:10 AM
This revision is now accepted and ready to land.Oct 6 2021, 7:10 AM
mgorny added inline comments.Oct 6 2021, 8:46 AM

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.

Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2021, 7:07 AM