[Lldb-commits] [PATCH] D44603: [lldb] Introduce StackFrameRecognizer

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This is good.  The addition of the "info" command will be helpful for people trying to debug their recognizers.  It's okay to add multiple -s and -n's later - though the fact that you don't allow "apply to all frames" may make us want the ability to provide more than one option sooner rather than later...

Sorry for this thought coming late, but I worry a little bit about the fact that the class name is the recognizers identity, particularly for deleting.  I might have a good recognizer class that I want to add to one library, then later in the session want to apply it to another library as well.  The second addition will mean that the name now exists twice but with different shared libraries, and then since I only provide the name to "delete" I can't tell which one I've deleted.

It also makes the API's a little odd, since the name of the recognizer is important to the commands for handling it, but it isn't clear from the API's that I have to provide a unique name for them.

I think it would be better to have the RecognizerManager keep an ID for each recognizer, and have list print and delete take the index.

Also, it might be convenient to have "frame recognizer delete" with no arguments query "Do you want to delete all recognizers" and then delete them.  That's the way "break delete" works so it's an accepted pattern in lldb.  This could be done as a follow-on, however.



================
Comment at: packages/Python/lldbsuite/test/functionalities/frame-recognizer/TestFrameRecognizer.py:54
+
+        self.runCmd("b foo")
+        self.runCmd("r")
----------------
Would you mind using:

lldbutil.run_break_set_by_symbol(self, "foo") 

instead of this runCmd.  The lldbutil routine will check that the breakpoint got set and error out here if it didn't.  That will reduce the head-scratching if for some reason we fail to set the breakpoint and then the test falls over downstream.



================
Comment at: packages/Python/lldbsuite/test/functionalities/frame-recognizer/TestFrameRecognizer.py:91
+        """
+        self.runCmd("b bar")
+        self.runCmd("c")
----------------
Also here.


https://reviews.llvm.org/D44603