Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add delay command #179

Merged
merged 1 commit into from
Dec 17, 2016
Merged

Add delay command #179

merged 1 commit into from
Dec 17, 2016

Conversation

zats
Copy link
Contributor

@zats zats commented Dec 11, 2016

This adds a delayed execution of commands for lldb

Reasoning

Often when debugging involved user interaction or interactive transitions in simulator, it takes some time to setup the interaction I need. Once setup, it is not unheard of that I can't switch away from simulator: if gesture-driven animation is in flight, switching away from simulator might interrupt it.

Usage

This is why I found it useful to do following:

$ zzz 3 "process interrupt" (== $ zzz 3)
$ zzz 3 pcomponents
etc…

This restarts execution, gives me 3 seconds to setup my interaction, and executes specified command afterwards.

My python is quite rusty, any feedback is highly appreciated!

@zats
Copy link
Contributor Author

zats commented Dec 11, 2016

Oh and if there is a built-in way of doing this, let me know 😆

@kastiglione
Copy link
Contributor

Looks legit, amazing name too. I want to give Chisel some aliasing features, in which case this could be aliased to delay as well. I have just a couple comments.

return 'Executes specified lldb command after delay.'

def options(self):
return []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be left out.


def run(self, arguments, options):
lldb.debugger.SetAsync(True)
lldb.debugger.HandleCommand('continue')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a Continue() function that can be called directly, but given the way Chisel is currently architected, doing it this way is cleaner. Wanna change this to 'process continue' to match the 'process interrupt' used below?

]

def run(self, arguments, options):
lldb.debugger.SetAsync(True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this code save the current value of async and set it back after?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like I didn't understand the meaning of SetAsync().
From documentation I thought that it detaches execution of the following command (continue in this case) allowing function to be evaluated even after we continue the execution.
Did I misunderstand it? Or maybe I'm just missing the point of your comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's right, but I didn't think its affect was for only one command, I'm thinking it stays in async mode until something calls SetAsync(False). If that is the case this command should change it back to the original value after it's done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually don't see it being used like that in other cases we either enable reset async conditionally or don't do it at all in FBFindCommands.py

I don't mind resetting if you are certain we should do it.
Sadly, no documentation, and I don't feel like lldb spelunking right now 😳

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spelunked last night, and the main thing is that the async setting decides whether Resume() or ResumeSynchronous() is called. I didn't write any code though which would really answer the question. I don't know if those other cases really confirm it either way.

Since it must work as is, we can leave this for another day 👍

@kastiglione
Copy link
Contributor

Ideally it wouldn't be necessary to put the command in quotes, but that can be a fix for another day.


def runDelayed(self, command):
lldb.debugger.HandleCommand('process interrupt')
lldb.debugger.HandleCommand(command)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does lldb silently accept an empty command? I ask because the description says zzz 3 "process interrupt" is equivalent to just zzz 3, so I'm presuming this means lldb doesn't error on empty commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually set a default value on the command option, it probably got lost because I didn't break it at certain line length (is there a style guide you'd like to follow here?)
The default parameter is , default='process interrupt'
Should I add a check for an empty command and replace it with default value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😎 I did overlook the default.

Given the declared default, don't feel strongly either way about handling an empty command.

@kastiglione
Copy link
Contributor

An interesting addition to this command some day is give it a "breakpoint equivalent" (function, line number, etc) and then the given command is run either when the breakpoint is hit, or when the delay/timeout happens.

Copy link
Contributor Author

@zats zats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re name: I actually looked for aliasing support; and yes, zzz might be a bit cryptic. I don't mind renaming it into delay for clarity.
Re adding breakpoint functionality: I'm not sure I understood the use case?


def runDelayed(self, command):
lldb.debugger.HandleCommand('process interrupt')
lldb.debugger.HandleCommand(command)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually set a default value on the command option, it probably got lost because I didn't break it at certain line length (is there a style guide you'd like to follow here?)
The default parameter is , default='process interrupt'
Should I add a check for an empty command and replace it with default value?

]

def run(self, arguments, options):
lldb.debugger.SetAsync(True)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like I didn't understand the meaning of SetAsync().
From documentation I thought that it detaches execution of the following command (continue in this case) allowing function to be evaluated even after we continue the execution.
Did I misunderstand it? Or maybe I'm just missing the point of your comment

@kastiglione
Copy link
Contributor

kastiglione commented Dec 13, 2016

Re the breakpoint idea, let me illustrate

delay "-[Foo didDoThing]" 3 pcomponents

Basically it's like a expiring breakpoint. Within the next three seconds, if the method is called, run pcomponents, otherwise call pcomponents when time runs out.

I guess the above is just three commands:

tbreak -[Foo didDoThing]
zzz 3 "break dis <bp>"
zzz 3 pcomponents

Which raises a question, can two or more zzz commands be run like that?

@zats
Copy link
Contributor Author

zats commented Dec 14, 2016

Hmm that's an interesting idea, but I'd probably extract it into a separate command or as an option to the existing one. Regardless seems like a material for another pull request. I'll make sure to fix other comments in the meantime

@kastiglione
Copy link
Contributor

Looks ready, you don't have any changes planned right?

@zats
Copy link
Contributor Author

zats commented Dec 16, 2016

Oh sorry, lost track of this PR. I didn't plan anything unless I forgot to address any of your comments?

@kastiglione kastiglione merged commit f0a8402 into facebook:master Dec 17, 2016
@kastiglione
Copy link
Contributor

Thanks!

@zats zats deleted the delay branch December 17, 2016 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants