-
Notifications
You must be signed in to change notification settings - Fork 349
Added support for finally blocks #196
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
Conversation
aecdc51 to
ddf1367
Compare
saelo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks for implementing this! Left some comments, should be good to merge after addressing those :)
|
I've addressed your review comments in the latest commit. Just a heads up, I have some concerns around the When the reducer tries to minimise this program, it attempts to Nop out the try block and the catch statement. So we end up with This generates invalid FuzzIL and fails static validation (due to v0 being re-defined) which triggers an assertion at runtime. I'm not sure if this is due to the bug in my code updates to the reducer (i.e. from the commit before addressing the review comments) or an issue elsewhere (e.g. should we Nop out the catch and finally blocks when reducing?). Thoughts? I'm going to run the fuzzer with the latest changes to this PR and see if I can recreate these crashes and report back, |
|
Ok interesting. So I think should already be invalid FuzzIL since v0 is declared two time (this should catch it). So I'm not sure what might have happened there. I'll take another look at the reducer, but I agree with you, running it for a bit and monitoring the outputs is probably the best approach here. |
4abd82b to
706d886
Compare
|
I've run the fuzzer built from commit ad7449d for a few hours with the following configuration and bumping up the weight on the I haven't noticed a crash/debug assertion related to try catch generation or reduction but the debug build did crash with the following assertion: From the stack trace it would seem that this is an issue unrelated to the current PR but it's probably worth investigating and tracking as a separate issue. I've rebased this PR with the latest changes from main so this should be good to merge. |
|
Cool! Yeah that crash indeed looks unrelated. It's unfortunately not really actionable without line numbers I guess, but if I get some time I'll try to reproduce it. Thanks! |
|
quick update on the crashes. I've upgraded my setup from Swift 5.4 to 5.4.1 and I no longer see crashes. I think this was a Swift quirk that was addressed with the update. Apologies, if I've set you on a wild goose chase. I don't think this requires any further investigation :) |
This PR adds support for finally blocks in try-catch statements.