Page MenuHomePhabricator

[Sema] Handle invalid noexcept expressions correctly.
AbandonedPublic

Authored by hintonda on Jan 3 2017, 3:00 PM.

Details

Summary

Treat invalid noexcept specifications in the same way we
treat invalid throw specifications, by not resetting the exception
type to EST_None, and testing isUsable instead of isInvalid.

Rational: In order to add source locations for exception
specifications (see https://reviews.llvm.org/D20428) we need to
maintain the correct exception type, otherwise, FunctionDecl types
won't include space for source ranges, but the associated
TypeSourceInfo type will, causing ASTContext::adjustExceptionSpec to
assert once source ranges have been added.

Event Timeline

hintonda updated this revision to Diff 82959.Jan 3 2017, 3:00 PM
hintonda retitled this revision from to [Sema] Handle invalid noexcept expressions correctly..
hintonda updated this object.
hintonda added a subscriber: cfe-commits.
rsmith added inline comments.Jan 3 2017, 3:25 PM
lib/Parse/ParseDeclCXX.cpp
3547 ↗(On Diff #82959)

Should NoexceptRange be set in the else case too, now that we're claiming that the type is EST_ComputedNoexcept?

lib/Sema/TreeTransform.h
5044–5057 ↗(On Diff #82959)

These changes don't make sense to me: if we get a valid-but-null ExprResult from any of the above, there is no guarantee a diagnostic has been produced, so it is not correct to return true.

Which call is producing the valid-but-null ExprResult?

hintonda added inline comments.Jan 3 2017, 4:45 PM
lib/Parse/ParseDeclCXX.cpp
3547 ↗(On Diff #82959)

If the expression is invalid, it's never used. In fact, since it is invalid, it may not be possible to compute the range. However, I'll check that.

lib/Sema/TreeTransform.h
5044–5057 ↗(On Diff #82959)

If the expression contains an undefined value, then ParseConstantExpression, and the functions it calls, will produce diagnostics and return an invalid ExprResult, i.e., 0x01. That means you can't call CheckBooleanCondition because it assumes the pointer is good. The problem is that this information is lost once tryParseExceptionSpecification returns. From then on, it's stored as a null value, i.e., as if it never existed which corresponds to EST_None.

However, we need to know that it did in fact exist so we can get the sizes of FunctionDecl type and TypeSourceInfo type to match, and that won't happen if we set the exception type to EST_None.

So, if we keep EST_ComputedNoexcept, we need to actually test the pointer before we use it -- isValid is no longer good enough.

Richard, what do you think about just avoiding the problem altogether by only calling actOnDelayedExceptionSpecification is noexcept type != EST_None?

hintonda updated this revision to Diff 83000.Jan 3 2017, 8:59 PM

Rollback previous change and instead only call
actOnDelayedExceptionSpecification if noexcept type is not EST_None.

rsmith edited edge metadata.Jan 5 2017, 3:44 PM

Looks OK. Is it possible to add a test case for this without D20428? If not, this is small enough that rolling it into D20428 (so it can be committed with its testcase) would make sense.

hintonda abandoned this revision.Jan 5 2017, 11:49 PM

Unable to test change here, so have included fix directly in D20428.