This is an archive of the discontinued LLVM Phabricator instance.

Fix an assertion failure in setPointOfInstantiation.
Needs ReviewPublic

Authored by hokein on Mar 1 2016, 5:54 AM.

Details

Summary

Fixes PR25668.

Diff Detail

Event Timeline

hokein updated this revision to Diff 49480.Mar 1 2016, 5:54 AM
hokein updated this revision to Diff 49481.
hokein retitled this revision from to Fix an assertion failure in setPointOfInstantiation..
hokein updated this object.

Update.

hokein set the repository for this revision to rL LLVM.
hokein added a subscriber: cfe-commits.
hokein added a comment.Mar 3 2016, 2:48 AM

friendly ping.

bkramer edited edge metadata.Mar 3 2016, 3:14 AM

This doesn't look right to me. Aborting tree transform on a bad source location seems just wrong. Where is the invalid source location coming from? Also the backtrace seems related to typo correction, maybe the bug is there?

hokein updated this revision to Diff 49939.Mar 7 2016, 1:59 AM

Update.

hokein added a comment.Mar 7 2016, 1:59 AM

This doesn't look right to me. Aborting tree transform on a bad source location seems just wrong. Where is the invalid source location coming from? Also the backtrace seems related to typo correction, maybe the bug is there?

I have post the whole stack trace in PR25668.
The invalid source location comes from TypoExpr which is returned by ArraySubscriptExpr->getLHS(), this is also why there is a FIXME in code line. After reading the code more throughly, I find that the invalid source location is expected when constructing a TypoExpr.

I updated the fixing patch now. We probably don't call RequireCompleteType with an invalid source location, which is the same as trunk@84793.

I'm still not sure if this is the right way to fix it. Maybe @rsmith has an opinion?

rsmith edited edge metadata.Mar 23 2016, 10:32 AM

It seems to me that the bug is that we're producing an invalid source location, not how we handle that downstream. Typo correction should be able to preserve the source location of whatever expression it was correcting.

lib/Sema/SemaExpr.cpp
14047–14048 ↗(On Diff #49939)

General rule: for a function that returns true to say "an error occurred", it is not acceptable to return true without either (a) producing an error or (b) observing some state that proves someone else emitted an error (such as a decl that has been set as invalid).