This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Don't issue analysis-based warnings when a fatal error has occurred
ClosedPublic

Authored by ahatanak on Oct 31 2016, 2:34 PM.

Details

Summary

This patch makes AnalysisBasedWarnings::IssueWarnings return without issuing warnings based on CFG analysis when a fatal error has occurred. This is needed to avoid a crash that occurs when compiling the test case (instantiate-template-fatal-error.cpp).

My first patch made changes in InstantiatingTemplate::InstantiatingTemplate to call hasUncompilableErrorOccurred instead of hasFatalErrorOccurred at SemaTemplateInstantiate.cpp:214. That fixed the crash for the test case but introduced a large number of regression test failures, so I fixed AnalysisBasedWarnings::IssueWarnings instead.

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 76467.Oct 31 2016, 2:34 PM
ahatanak retitled this revision from to [Sema] Don't issue analysis-based warnings when a fatal error has occurred.
ahatanak updated this object.
ahatanak added reviewers: rsmith, majnemer.
ahatanak added a subscriber: cfe-commits.
rsmith edited edge metadata.Oct 31 2016, 5:49 PM

It looks like the bug is that if we hit an uncompilable error after hitting a non-uncompilable fatal error (promoted from a warning), we fail to notice that we've hit an uncompileable error. Our AST should be complete if hasUncompilableErrorOccurred() is not set, and it looks like we're violating that invariant here.

ahatanak updated this revision to Diff 76513.Oct 31 2016, 6:31 PM
ahatanak edited edge metadata.

I think the non-uncompilable error you mentioned is the one on line 7 of the test case and the uncompilable error is the one on line 15. Is that correct? The modified test case has only one error (on line 7, which is promoted from a warning) and it compiles without any errors if the #pragmas are removed.

It seems to me that the root of the problem is that the template instantiation S2<int> is left in an incomplete state and VarDecl s2 in the AST, which is of type S2<int>, is marked invalid. As I mentioned in my previous comment, I tried to fix this by calling hasUncompilableErrorOccurred instead of hasFatalErrorOccurred in InstantiatingTemplate::InstantiatingTemplate, but that didn't seem correct as a large number of regression tests failed. What is the right way to fix this?

`-FunctionDecl 0x7f889602fa90 <line:19:1, line:22:1> line:19:6 foo1 '_Bool (const long long *, int *)'

|-ParmVarDecl 0x7f889602f918 <col:11, col:28> col:28 used a 'const long long *'
|-ParmVarDecl 0x7f889602f9c0 <col:31, col:36> col:36 used b 'int *'
`-CompoundStmt 0x7f889602fde0 <col:39, line:22:1>
  `-DeclStmt 0x7f889602fd78 <line:20:3, col:13>
    `-VarDecl 0x7f889602fd18 <col:3, col:11> col:11 invalid s2 'S2<int>':'struct S2<int>'

It seems to me that the root of the problem is that the template instantiation S2<int> is left in an incomplete state and VarDecl s2 in the AST, which is of type S2<int>, is marked invalid. As I mentioned in my previous comment, I tried to fix this by calling hasUncompilableErrorOccurred instead of hasFatalErrorOccurred in InstantiatingTemplate::InstantiatingTemplate, but that didn't seem correct as a large number of regression tests failed. What is the right way to fix this?

Ah, I see. Try changing that to hasFatalErrorOccurred() && hasUncompilableErrorOccurred(). (The former indicates we don't need more diagnostics, and the latter indicates we don't need a complete AST; if both flags are set, then we don't need to instantiate any more templates.)

ahatanak updated this revision to Diff 76521.Oct 31 2016, 10:40 PM

Thanks, that fixed the failing regression tests.

rsmith accepted this revision.Nov 2 2016, 6:47 PM
rsmith edited edge metadata.
rsmith added inline comments.
lib/Sema/SemaTemplateInstantiate.cpp
213–214

Maybe "Any diagnostics we might have raised will not be visible, and we do not need to construct a correct AST." to justify the two checks?

This revision is now accepted and ready to land.Nov 2 2016, 6:47 PM
This revision was automatically updated to reflect the committed changes.
malcolm.parsons added inline comments.
cfe/trunk/lib/Sema/SemaTemplateInstantiate.cpp
213 ↗(On Diff #76863)

s/occcured/occurred/