Page MenuHomePhabricator

[LLDB] Add per-thread register infos shared pointer in gdb-remote

Authored by omjavaid on Jun 30 2020, 3:46 AM.



In gdb-remote process we have register infos defind GDBRemoteDynamicRegisterInfo reference. In past register infos have remained constant througout the life time of a process.

This changes in AArch64 SVE variant where register infos will have per-thread configuration. SVE register will have per-thread size and can be updated while running. This patch aims to build up for that support by change GDBRemoteDynamicRegisterInfoSP from a reference defined in GDBRemoteProcess to a shared pointer deinfed per-thread.

Diff Detail

Event Timeline

omjavaid created this revision.Jun 30 2020, 3:46 AM
labath added a comment.Sep 3 2020, 5:02 AM

There are things I'd want to change here, but I think this is patch is fundamentally ok. Before going into that, I think we should first sort out the dependant patch.

omjavaid updated this revision to Diff 304728.Nov 11 2020, 11:34 PM

Minor update after changes to D82863.

omjavaid updated this revision to Diff 308340.Nov 30 2020, 5:45 AM

Updated after re-base over D91241.

labath added inline comments.Dec 1 2020, 5:40 AM



nit: std::move(reg_info) (and changing the uses below to m_reg_info_sp).


This is basically making a copy, right? Could we just use the copy constructor for this (maybe coupled with declaring the class final to avoid the possibility of slicing)?


How about:

if (!force &&  m_register_info_sp)
m_register_info_sp = std::make_shared<GDBRemoteDynamicRegisterInfo>();

Is this going to have more callers in the future? It seems that things would be a lot simpler if we just did this directly in the constructor...

omjavaid updated this revision to Diff 308863.Dec 1 2020, 8:48 PM

Updated after incorporating suggested changes.

omjavaid updated this revision to Diff 308932.Dec 2 2020, 4:09 AM

Added copy constructor.

rovka added a subscriber: rovka.Dec 4 2020, 2:03 AM
rovka added inline comments.

I think operator= should be default too if the copy constructor is default.

labath added inline comments.Dec 4 2020, 2:43 AM

Yeah, we can make both of them protected to avoid accidental use.

rovka added a comment.Dec 4 2020, 3:56 AM

Sorry, I had one more comment :) I think that should be all from my side.


Is it still necessary to call Clear? I think the default constructor should give us a "cleared" object.

labath added inline comments.Dec 7 2020, 12:04 AM

Can't you just use the process constructor argument here? (That's what makes this code simpler vs the separate function approach)


Is this ever false?

omjavaid updated this revision to Diff 309860.Dec 7 2020, 3:27 AM

This update resolves review comments from @rovka and @labath

@labath Is this ok for commit?

labath accepted this revision.Dec 10 2020, 12:52 AM

looks good

This revision is now accepted and ready to land.Dec 10 2020, 12:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2021, 3:11 AM