[Lldb-commits] [PATCH] D11094: Refactor Unix signals.

[Greg Clayton]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

So what is the overall goal of this patch? I am assuming the jist of this patch is to just get the JSON packet to get unix signals from the remote GDB server.

The following things MUST be true:

- Each process must have its own unix signals, i.e. they must be separate UnixSignals instances. Different processes might setup which signals to ignore and pass along and notify differently and this shouldn't change.
- You can't add ivars to lldb::SB classes there must be one shared_ptr, unique_ptr or raw pointer and its size must match what was there before for API compatability

I would rather not see SBPlatform get UnixSignals if we can help it. It would be nice from the standpoint that you might be able to grab a copy of them and modify them before any processes are launched, but I don't see us doing any of this anywhere so I would rather avoid it until we do. The theory is you can modify a platforms unix signals, and then each process will get a copy of the current platforms signals when the process is created, but each process needs to have its own instance of UnixSignals so each process can set which signals to stop/notify/continue from separately, all processes can't use the same instance otherwise you make other processes stop when they didn't want to for specified signals. We have lldb.util.get_signal_number() that is trying to take advantage of the platform having unix signals but I would argue that we should have a lldb.SBProcess available anytime we need this information, so the platform doesn't really need signals right now. So I believe Process should still have a m_unix_signals object that is owned by it just like it used to and it can be seeded with startup values if needed.

So what I don't like about the platform having unix signals and this patch:

- Each platform might be able to guess what signals it has by making a constant copy of some signals from some version of the their previous OS's but this can get out of date
- Each platform could provide the correct signals if it is connected to a remote platform, but connection to a platform isn't required (for iOS we don't connect to a remote platform) and the lldb-server on the other side might not support the JSON signal packet, and if not then we are back to guessing as in the above step
- We can't share unix signals between multiple processes, they each need to have their own instance

What I do like about the platform having unix signals:

- We can get ahold of SBUnixSignals before we run and set up signals so each process that gets launched can init itself with a copy of one set of settings (but this patch doesn't do that).

A quick not about public API stuff:

- Any changes in the public API must be additions only and no changes to previous API can be made
- no ivar size changes
- Don't bother with any "const" specifications on ANY methods as they are useless. The only thing it does it makes sure the shared_ptr, unique_ptr or pointer can't be changed. We have some functions that people put "const" on erroneously, but we are stuck with it now since we can't change public API. We shouldn't add more.

So let me know what you think after reading my above comments. I would almost rather this patch just do the following:

- introduce the new JSON unix signal packet and use it for ProcessGDBRemote
- Don't do any of the SBPlatform stuff, the only thing we are using it for is lldbutil.get_signal_number() and we should be able to use the current process for this
- Backout all public API changes
- Backout all host changes


================
Comment at: include/lldb/API/SBUnixSignals.h:77-78
@@ -77,3 +76,4 @@
 private:
-    lldb::ProcessWP m_opaque_wp;
+    lldb::UnixSignalsSP m_opaque_sp;
+    lldb::ProcessWP m_process_wp;
 };
----------------
All objects in the public API must never change size so you can't add one here. If clients update LLDB they should still be able to link against the LLDB public interface without anything changing, adding an ivar changes the byte size, anyone that might have inherited from or use a SBUnixSignal as an ivar, will now crash.

================
Comment at: include/lldb/Host/Host.h:248-249
@@ -246,4 +247,4 @@
 
-    static const lldb_private::UnixSignalsSP&
-    GetUnixSignals ();
+    static const lldb::UnixSignalsSP &
+    GetUnixSignals(const ArchSpec &arch = HostInfo::GetArchitecture());
 
----------------
I am confused as to why this change was made. In the host layer there should only be 1 host set of unix signals. If you are trying to make a grab bag for any kind of canned unix signals, please make a static function in the lldb_private::UnixSignals class.


http://reviews.llvm.org/D11094