[Lldb-commits] [lldb] [LLDB] Add module hook implementation (PR #185465)

[via lldb-commits]

jimingham wrote:

Those were good changes.  I have a couple specific objections, but again this is close:

1) You should treat the triggers seriously for both command and script hooks.  That means:
  a) Compute the triggers when you are handed a class.  You should be able to do that pretty easily since it's just looking up three methods in the python class.  That will also allow you to show users what they should have provided if they misspell the method names.
  b) Don't allow the trigger mask for scripted classes to be modified, that should be a command error.  The Set methods for triggers should either only exist on HookCommand, or return an error so that the scripted hooks can tell you you can't do that to them.

2) Separate hook `enable/disable` from hook `modify`.  I think the commands will be easier to use if you make that distinction.  Also with that division you could also later on provide `target hook modify --command "new command" 8` and change filters, etc., which I am pretty sure we will want to do.  For the command version particularly I can see building up the hook as you are playing with it, and it would be nice not to have to recreate the current hook every time to be able to add to it.  Not that you should do that as part of this patch.  But by making that separation now we won't have to change how enable/disable work to support it.

3) You don't have a test that a stop-hook added your way works.  You probably could just modify one of the current stop-hook tests to have two ways of setting the hook, since the hooks work the same way, so you can reuse all of the test except adding the hook.

I made some inline comments as well, though in the end they boil down to these three.


https://github.com/llvm/llvm-project/pull/185465