The crux of the bug is described in a code-comment (CSE stands for common subexpression elimination):
> As n is known to be negative here, this ArithAbs will become a ArithNegate. That negation will be checked, but then be CSE'd for the previous, unchecked one. This is the compiler bug.
To be clear, the ArithNegate implementation in CSE that had the bug, as its implementation failed to take into account that the current mode might require checking for an overflow (which is required for the tricky case of -INT_MAX)–perhaps a typo or a copy/paste oversight. I wonder if anyone (clearly not the JavaScriptCore team) has done the work to lift the semantics of these optimizations to something formal and prove that they are correct…
As Groß correctly states in his post, the bug was in how clobberize models that instruction. It’s not that this is a bug in CSE, since this analysis is shared by (many) other passes.
It’s sort of also a bug in the IR design. The fact that opcodes have modes has always caused bugs. It’s too easy to forget about that when modeling.
But that structure - separation of modeling and CSE - is designed to make the whole thing easier to reason about despite the underlying semantics being quite nasty.
It’s weird to say that JSC team clearly wouldn’t try to prove this right by lifting the semantics out or what have you. What makes you say that?
> It’s not that this is a bug in CSE, since this analysis is shared by (many) other passes.
Is DFGClobberize not part of CSE? I read the post and glanced at the code and I’m not sure if I understand your clarification otherwise.
Do note that my comment is not the team “clearly wouldn’t try to prove this right” but is really “hasn’t proved this right”, the “clearly” part being a reference to this bug ;) I would be extremely surprised if there hasn’t been work done by the team to partially verify instruction side effects given how often they lead to bugs in the JIT (I haven’t seen much writing much about it, though, so if there’s something on the WebKit blog about this I’d love to give it a read). However, what I’m suggesting is some sort of proof that the semantics encoded in JavaScriptCore are complete and correct; maybe with some formal language specification involved. Or maybe you could have the CI machines churn through the entire state space for the unary instructions during the holidays or something :)
Making the arithmode argument optional seems like an avoidable mistake here. If it was always required to explicitly specify it, it would have been more visible to a reviewer's eye that the problem opcode was an outlier.
