ConstraintAnalysis: Fix a hang with a tee in an increment - #9058
Conversation
| if (auto* binary = set->value->dynCast<Binary>()) { | ||
| if (binaryActionCounts[binary]++ >= MaxBinaryActions) { | ||
| constraints.setProvesNothing(set->index); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why wasn't this catch-all widening sufficient to prevent the problem? Because the expression we have here isn't a Binary? Could we fix it by just handling any kind of expression here instead of just Binary?
There was a problem hiding this comment.
The issue was that we didn't handle this wrt a fallthrough. That is, we computed the fallthrough after, perhaps producing a Binary then - after this check.
That was silly and also inefficient...
| // work. | ||
| break; | ||
| } | ||
| auto* next = Properties::getImmediateFallthrough( |
There was a problem hiding this comment.
Isn't there an optional parameter to getFallthrough that will give us this NoTee behavior without us having to reimplement the loop?
There was a problem hiding this comment.
There is, but it also skips BrIfs:
Line 271 in 39be64e
We could perhaps make that flag more refined, but I'm not sure it's worth it.
|
|
||
| // Now that we know the value, check binary action counting limits (see | ||
| // above). | ||
| if (auto* binary = value->dynCast<Binary>()) { |
There was a problem hiding this comment.
Is there any reason to continue restricting this to Binary?
There was a problem hiding this comment.
I'd like to keep this "precise" to the things it needs to handle. There is nothing else where we have this issue other than a Binary, atm (and I'm not sure we ever will?)
There was a problem hiding this comment.
Yeah, I guess all infinite analysis loops would involve a Binary expression somewhere. But it seems somewhat arbitrary to attach the counter to only Binary expressions. Maybe instead of counting the number of times we analyze particular expressions, we can count the number of times we analyze each basic block. That would be even more precise and less arbitrary. I guess we could do that in a follow-up, though.
There was a problem hiding this comment.
To me it makes sense as a Binary is where x = x + 1 happens (the + 1). Other things only flow around data.
With that said, maybe it is nicer to count basic block operations? Worth thinking about.
2 participants
No description provided.