Fix catch-all exception handlers #1435
Conversation
|
Thanks, I'll give it a try today. I just wasted 19h of CPU time on our server because of this issue again. |
|
It worked at least on my machine, I'll give it another try on the server once it's done running benchmarks. |
|
I made my best to avoid catching I also made the timeout timer more robust: it will keep resending the signal every second. So even if execution happens to be inside a catch-all exception handler at any point (in the future), then there will be more opportunities for the special exception. Currently, if it's swallowed once, it will never come again. |
|
The state as of a35dc82 does not fix the issue on our server. I'll try again with the new commits added. |
| try Addr.type_of (choose xs) | ||
| with (* WTF? Returns TVoid when it is unknown and stuff??? *) | ||
| | _ -> voidType | ||
| Addr.type_of (choose xs) (* TODO: what if ambiguous type? what if chooses NullPtr but also contains Addr with proper type? *) |
There was a problem hiding this comment.
Are we sure it's safe to remove the void fallback for Not_found?
There was a problem hiding this comment.
I just moved it to two call sites in a35dc82, where this otherwise lead to crashes on regression tests. And even those are kind of dubious: why should we ever try to get the type of an empty points-to set?
Even so, the worst that can now happen is that there's an uncaught Not_found (if that ever happens), whose stacktrace is easy to see and the fallback can easily be added to such call sites. I wouldn't do that en masse because it itself would cover up problems where we try to operate on empty points-to set when they should've already been handled.
There was a problem hiding this comment.
I am more worried about the Addr.type_of throwing then providing an empty points-to-set.
There was a problem hiding this comment.
Where is the empty points-to set being provided now?
It was the old behavior which returned the same result for type errors as it did for empty points-to sets, making the results indistinguishable.
Now it's up to the caller to handle whichever exception this raises if the caller provides incorrectly constructed addresses.
Making every invalid offset appear to have void type is hardly the right thing.
There was a problem hiding this comment.
Making every invalid offset appear to have
voidtype is hardly the right thing
I think it may be crucial for not crashing on some of our real-world programs. I think we should do a run for those after merging this.
There was a problem hiding this comment.
Sure, I don't added call-site catching where it was needed for our test suite. If some Not_found or Type_of_error now slips through, it will be uncaught and crashes Goblint with a stacktrace, so it's quite easy to see which call of AD.type_of should have a fallback (or better decide what on the result).
So, yes, if they come up (on sv-benchmarks or our bench stuff), we can easily put in such specific catch.
This is an attempt to fix #1420 (comment).
@michael-schwarz You may want to try out of this helps with #1420.