This is an archive of the discontinued LLVM Phabricator instance.

[Parser] Fix a crash on invalid where a delayed TypoExpr was corrected twice
ClosedPublic

Authored by erik.pilkington on May 20 2016, 2:02 PM.

Details

Summary

Previously, Clang crashed when parsing a BinaryOperator in C with a typo when a typo was already found. This is because the parser called Sema::ActOnBinOp, which corrects the found typo in C mode, then corrects the typo again from the parser. During the first correction pass, the TypoExprState corresponding to the typo was cleared from Sema when it was corrected. During a second pass, an assert fails in Sema::getTypoExprState because it cannot find the TypoExprState. The fix is to avoid correcting delayed typos in the parser in that case.

This patch looks like it fixes PR26700, PR27231, and PR27038.

On a more general note, the handling of delayed typo expressions is very messy right now, some of them are handled in semantic analysis, and some are handled in the parser, leading to easy to make responsibility bugs like this one. I think I might take a look at moving the correcting to one side or the other in a future patch.

Diff Detail

Repository
rL LLVM

Event Timeline

erik.pilkington retitled this revision from to [Parser] Fix a crash on invalid where a delayed TypoExpr was corrected twice.
erik.pilkington updated this object.
erik.pilkington added a reviewer: rsmith.
erik.pilkington added a subscriber: cfe-commits.
rsmith added inline comments.Jun 9 2016, 1:27 PM
lib/Parse/ParseExpr.cpp
450–452 ↗(On Diff #57980)

The inconsistent behavior of ActOnBinOp seems somewhere between an implementation detail and a bug; it doesn't seem reasonable for the parser to rely on that. I'm not particularly happy about making changes like this without some documentation of the overall design that shows whose responsibility it is to correct typos in which cases.

Before we introduced TypoExpr, the parser was permitted to simply discard Expr nodes that it didn't use (because it'd hit a parse error). Ideally, I'd like to return to that state of affairs, by removing the relevant CorrectDelayedTyposInExpr calls from the parser and having Sema automatically diagnose them when we get to the end of the relevant context, if we've not already done so.

Another reasonable-seeming option would be to add a Sema::ActOnDiscardedExpr(Expr*) that the parser can call (which calls CorrectDelayedTyposInExpr), and make it clear that the parser is responsible for passing each Expr that it receives from Sema to exactly one ActOn* function (unless otherwise specified) -- that way, at least the responsibilities will be clear, but it doesn't help us avoid bugs where TypoExprs are accidentally discarded.

rsmith accepted this revision.Jun 13 2016, 1:22 PM
rsmith edited edge metadata.

Let's go ahead with this for now and figure out the proper way to handle this as a follow-up change.

This revision is now accepted and ready to land.Jun 13 2016, 1:22 PM
This revision was automatically updated to reflect the committed changes.