Re: [PATCH] D20490: [Parser] Fix a crash on invalid where a delayed TypoExpr was corrected twice

2016-06-13 Thread Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL272587: [Parser] Only correct delayed typos when needed 
(authored by epilk).

Changed prior to commit:
  http://reviews.llvm.org/D20490?vs=57980=60603#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20490

Files:
  cfe/trunk/lib/Parse/ParseExpr.cpp
  cfe/trunk/test/Sema/typo-correction.c

Index: cfe/trunk/test/Sema/typo-correction.c
===
--- cfe/trunk/test/Sema/typo-correction.c
+++ cfe/trunk/test/Sema/typo-correction.c
@@ -57,3 +57,11 @@
 }
 
 int d = X ? d : L; // expected-error 2 {{use of undeclared identifier}}
+
+int fn_with_ids() { ID = ID == ID >= ID ; } // expected-error 4 {{use of 
undeclared identifier}}
+
+int fn_with_rs(int r) { r = TYPO + r * TYPO; } // expected-error 2 {{use of 
undeclared identifier}}
+
+void fn_with_unknown(int a, int b) {
+  fn_with_unknown(unknown, unknown | unknown); // expected-error 3 {{use of 
undeclared identifier}}
+}
Index: cfe/trunk/lib/Parse/ParseExpr.cpp
===
--- cfe/trunk/lib/Parse/ParseExpr.cpp
+++ cfe/trunk/lib/Parse/ParseExpr.cpp
@@ -446,6 +446,10 @@
 
 LHS = Actions.ActOnBinOp(getCurScope(), OpToken.getLocation(),
  OpToken.getKind(), LHS.get(), RHS.get());
+
+// In this case, ActOnBinOp performed the CorrectDelayedTyposInExpr 
check.
+if (!getLangOpts().CPlusPlus)
+  continue;
   } else {
 LHS = Actions.ActOnConditionalOp(OpToken.getLocation(), ColonLoc,
  LHS.get(), TernaryMiddle.get(),


Index: cfe/trunk/test/Sema/typo-correction.c
===
--- cfe/trunk/test/Sema/typo-correction.c
+++ cfe/trunk/test/Sema/typo-correction.c
@@ -57,3 +57,11 @@
 }
 
 int d = X ? d : L; // expected-error 2 {{use of undeclared identifier}}
+
+int fn_with_ids() { ID = ID == ID >= ID ; } // expected-error 4 {{use of undeclared identifier}}
+
+int fn_with_rs(int r) { r = TYPO + r * TYPO; } // expected-error 2 {{use of undeclared identifier}}
+
+void fn_with_unknown(int a, int b) {
+  fn_with_unknown(unknown, unknown | unknown); // expected-error 3 {{use of undeclared identifier}}
+}
Index: cfe/trunk/lib/Parse/ParseExpr.cpp
===
--- cfe/trunk/lib/Parse/ParseExpr.cpp
+++ cfe/trunk/lib/Parse/ParseExpr.cpp
@@ -446,6 +446,10 @@
 
 LHS = Actions.ActOnBinOp(getCurScope(), OpToken.getLocation(),
  OpToken.getKind(), LHS.get(), RHS.get());
+
+// In this case, ActOnBinOp performed the CorrectDelayedTyposInExpr check.
+if (!getLangOpts().CPlusPlus)
+  continue;
   } else {
 LHS = Actions.ActOnConditionalOp(OpToken.getLocation(), ColonLoc,
  LHS.get(), TernaryMiddle.get(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20490: [Parser] Fix a crash on invalid where a delayed TypoExpr was corrected twice

2016-06-13 Thread Richard Smith via cfe-commits 
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

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


http://reviews.llvm.org/D20490



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20490: [Parser] Fix a crash on invalid where a delayed TypoExpr was corrected twice

2016-06-09 Thread Richard Smith via cfe-commits 
rsmith added inline comments.


Comment at: lib/Parse/ParseExpr.cpp:450-452
@@ -449,1 +449,5 @@
+
+// In this case, ActOnBinOp performed the CorrectDelayedTyposInExpr 
check.
+if (!getLangOpts().CPlusPlus)
+  continue;
   } else {

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 `TypoExpr`s are accidentally 
discarded.


http://reviews.llvm.org/D20490



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20490: [Parser] Fix a crash on invalid where a delayed TypoExpr was corrected twice

2016-06-09 Thread Erik Pilkington via cfe-commits 
erik.pilkington added a comment.

Pong!!


http://reviews.llvm.org/D20490



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20490: [Parser] Fix a crash on invalid where a delayed TypoExpr was corrected twice

2016-06-02 Thread Erik Pilkington via cfe-commits 
erik.pilkington added a comment.

Ping!!


http://reviews.llvm.org/D20490



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D20490: [Parser] Fix a crash on invalid where a delayed TypoExpr was corrected twice

2016-05-20 Thread Erik Pilkington via cfe-commits 
erik.pilkington created this revision.
erik.pilkington added a reviewer: rsmith.
erik.pilkington added a subscriber: cfe-commits.

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.

http://reviews.llvm.org/D20490

Files:
  lib/Parse/ParseExpr.cpp
  test/Sema/typo-correction.c

Index: test/Sema/typo-correction.c
===
--- test/Sema/typo-correction.c
+++ test/Sema/typo-correction.c
@@ -57,3 +57,11 @@
 }
 
 int d = X ? d : L; // expected-error 2 {{use of undeclared identifier}}
+
+int fn_with_ids() { ID = ID == ID >= ID ; } // expected-error 4 {{use of 
undeclared identifier}}
+
+int fn_with_rs(int r) { r = TYPO + r * TYPO; } // expected-error 2 {{use of 
undeclared identifier}}
+
+void fn_with_unknown(int a, int b) {
+  fn_with_unknown(unknown, unknown | unknown); // expected-error 3 {{use of 
undeclared identifier}}
+}
Index: lib/Parse/ParseExpr.cpp
===
--- lib/Parse/ParseExpr.cpp
+++ lib/Parse/ParseExpr.cpp
@@ -446,6 +446,10 @@
 
 LHS = Actions.ActOnBinOp(getCurScope(), OpToken.getLocation(),
  OpToken.getKind(), LHS.get(), RHS.get());
+
+// In this case, ActOnBinOp performed the CorrectDelayedTyposInExpr 
check.
+if (!getLangOpts().CPlusPlus)
+  continue;
   } else {
 LHS = Actions.ActOnConditionalOp(OpToken.getLocation(), ColonLoc,
  LHS.get(), TernaryMiddle.get(),


Index: test/Sema/typo-correction.c
===
--- test/Sema/typo-correction.c
+++ test/Sema/typo-correction.c
@@ -57,3 +57,11 @@
 }
 
 int d = X ? d : L; // expected-error 2 {{use of undeclared identifier}}
+
+int fn_with_ids() { ID = ID == ID >= ID ; } // expected-error 4 {{use of undeclared identifier}}
+
+int fn_with_rs(int r) { r = TYPO + r * TYPO; } // expected-error 2 {{use of undeclared identifier}}
+
+void fn_with_unknown(int a, int b) {
+  fn_with_unknown(unknown, unknown | unknown); // expected-error 3 {{use of undeclared identifier}}
+}
Index: lib/Parse/ParseExpr.cpp
===
--- lib/Parse/ParseExpr.cpp
+++ lib/Parse/ParseExpr.cpp
@@ -446,6 +446,10 @@
 
 LHS = Actions.ActOnBinOp(getCurScope(), OpToken.getLocation(),
  OpToken.getKind(), LHS.get(), RHS.get());
+
+// In this case, ActOnBinOp performed the CorrectDelayedTyposInExpr check.
+if (!getLangOpts().CPlusPlus)
+  continue;
   } else {
 LHS = Actions.ActOnConditionalOp(OpToken.getLocation(), ColonLoc,
  LHS.get(), TernaryMiddle.get(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits