Page MenuHomePhabricator

Make ProcessGDBRemote get a //copy// of platform Unix signals.
ClosedPublic

Authored by chaoren on Aug 27 2015, 6:07 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

chaoren retitled this revision from to Make ProcessGDBRemote get a //copy// of platform Unix signals..
chaoren updated this object.
chaoren added reviewers: clayborg, jaydeep.
chaoren added a subscriber: lldb-commits.
tberghammer added inline comments.
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
853 ↗(On Diff #33386)

I think it would be better to change SetUnixSignals to create the copy (or force the client side copy with some magic with rvalue references) because this error will happen almost every time somebody calls SetUnixSignals.

labath added a subscriber: labath.Aug 28 2015, 5:38 AM
labath added inline comments.
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
853 ↗(On Diff #33386)

If the platform is supposed to hold signals, which are not to be changed by its users, I would have it hand out std::shared_ptr<CONST UnixSignals> (or even const UnixSignals & ?). Then the users will be forced to make a copy if they want to do anything to them.

  • Force client side copy.
chaoren marked an inline comment as done.Aug 28 2015, 1:54 PM
chaoren added inline comments.
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
853 ↗(On Diff #33386)

If the platform is supposed to hold signals, which are not to be changed by its users

I think in my original discussion with Greg, we decided that it would be a good idea if we allow setting suppress/stop/notify for the platform signal set, and will be inherited by any process signal set created from it.

  • ProcessGDBRemote should automatically get a copy of GDBRemoteSignals.
clayborg edited edge metadata.Aug 31 2015, 11:09 AM

We do want processes to have individual copies that are copied from the platform. Does this actually make a copy? I don't see the copy.

chaoren added inline comments.Aug 31 2015, 1:47 PM
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
2910 ↗(On Diff #33485)

Here's the copy.

clayborg accepted this revision.Sep 1 2015, 9:48 AM
clayborg edited edge metadata.
This revision is now accepted and ready to land.Sep 1 2015, 9:48 AM
This revision was automatically updated to reflect the committed changes.