Page MenuHomePhabricator

[Parser] Avoid correcting delayed typos in array subscript multiple times.
ClosedPublic

Authored by vsapsai on Apr 17 2019, 7:05 PM.

Details

Summary

We correct some typos in ActOnArraySubscriptExpr and
ActOnOMPArraySectionExpr, so when their result is ExprError, we can
end up correcting delayed typos in the same expressions again. In
general it is OK but when NumTypos is incorrect, we can hit the
assertion

Assertion failed: (Entry != DelayedTypos.end() && "Failed to get the state for a TypoExpr!"), function getTypoExprState, file clang/lib/Sema/SemaLookup.cpp, line 5219.

This assertion is reproducible with Objective-C method

- (void)test {
  [self undeclaredMethod:undeclaredArg];
  NSMutableDictionary *opts = [NSMutableDictionary new];
  opts[(__bridge id)undeclaredKey] = @0;
}

There is no test for the fix because NumTypos is still incorrect and
we hit the assertion

Assertion failed: (DelayedTypos.empty() && "Uncorrected typos!"), function ~Sema, file clang/lib/Sema/Sema.cpp, line 382.

Another option is to fix tracking delayed typos but it is non-trivial as
in many cases we drop erroneous expressions without cleaning up
TypoExpr they contain. Also the assertion in ~Sema isn't causing
problems with assertions disabled, while a missing TypoExprState can
cause a segmentation fault.

rdar://problem/47403222

Diff Detail

Repository
rL LLVM

Event Timeline

vsapsai created this revision.Apr 17 2019, 7:05 PM
vsapsai edited the summary of this revision. (Show Details)Apr 17 2019, 7:14 PM

Wait, why is NumTypos incorrect here? I think its because we don't handle the typo on the line: [self undeclaredMethod:undeclaredArg];, even the following asserts now. Seems like the right fix would be to track down why we aren't handling the typo in the message expr.

// RUN: clang -cc1 %s -fobjc-arc
@implementation X
-x { [self undeclaredMethod:undeclaredArg]; }
@end

Wait, why is NumTypos incorrect here? I think its because we don't handle the typo on the line: [self undeclaredMethod:undeclaredArg];, even the following asserts now. Seems like the right fix would be to track down why we aren't handling the typo in the message expr.

// RUN: clang -cc1 %s -fobjc-arc
@implementation X
-x { [self undeclaredMethod:undeclaredArg]; }
@end

The reason why we aren't handling the typo in this case is because TypoExpr is created for undeclaredArg and when we emit err_arc_may_not_respond we just return ExprError and drop MultiExprArg without checking if it contains TypoExpr or not. It is possible to fix this case but we have many more. Some of them are

@class Test;
void test(void) {
  Test *obj;
  [obj test:undeclaredArg];
}
struct S {
  int x;
} S;
void test(void) {
  [S test:undeclaredArg];
}

I think that with the current design for delayed typos it's not feasible to fix all of these lingering delayed typos. Given that with disabled assertions non-empty DelayedTypos in ~Sema isn't causing crashes, I've decided to improve handling the subscripts.

erik.pilkington accepted this revision.Apr 25 2019, 5:23 PM

Huh, okay. I guess there are two separate bugs that are conspiring to crash the compiler here: we shouldn't correct a TypoExpr more than once, and we shouldn't correct a TypoExpr less than once. I think fixing one of the two is fine. FYI I think you should be able to write a testcase if you RUN it with -disable-free.

This revision is now accepted and ready to land.Apr 25 2019, 5:23 PM
vsapsai updated this revision to Diff 197421.Apr 30 2019, 1:04 PM
  • Add a test case with -disable-free.

Thanks for the suggestion, Erik.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2019, 12:23 PM