This is an archive of the discontinued LLVM Phabricator instance.

[CFG] Fix an assertion failure with static initializers
ClosedPublic

Authored by xazax.hun on Dec 20 2019, 5:29 PM.

Details

Summary

The branches protecting the static initializers have a DeclStmt as last Stmt. Since it is not an expression that will trigger an assertion failure in CFGBlock::getLastCondition.
This is triggered by the lifetime analysis that is currently in a fork. I am not sure what would be the best way to test this change here.

Diff Detail

Event Timeline

xazax.hun created this revision.Dec 20 2019, 5:29 PM
Charusso added inline comments.Dec 20 2019, 5:40 PM
clang/lib/Analysis/CFG.cpp
5923

What about the following?:

if (const auto *E = dyn_cast<Expr>(StmtElem->getStmt()))
  return E->IgnoreParens();
return nullptr;

I am not sure what would be the best way to test this change here.

I'll admit, I'm a bit out of touch with Clang and will unfortunately be for a while, but the short answer is that the unit tests we have for the CFG are kinda bad. The constructed CFG has a longer lifetime than the AST its based on (see: D63538), so we would need to rewrite the BuildCFG() function that creates it. I have some memories digging into it but always got distracted :)

Until then, could you please include the crash you mentioned through a regular lit test?

clang/lib/Analysis/CFG.cpp
5925–5926

I guess its time to update and/or move this comment a few lines up, maybe even add a descriptive assert.

NoQ added a comment.Dec 21 2019, 7:31 PM

I am not sure what would be the best way to test this change here.

A unittest, if you're brave :)

clang/lib/Analysis/CFG.cpp
5923

Not fail-fast enough.

NoQ added a comment.Dec 22 2019, 6:11 AM

Wait, i didn't real @Szelethus's reply :)

In any case, i'd love to see an example of such CFG and whether control dependency tracking can expose the crash.

xazax.hun updated this revision to Diff 235161.Dec 23 2019, 9:57 AM

I decided to fix the unittests. Having CFGs full of dangling pointers to the AST does not look fun at all, so I think this change was long overdue.

Szelethus accepted this revision.Dec 23 2019, 2:50 PM

I decided to fix the unittests. Having CFGs full of dangling pointers to the AST does not look fun at all, so I think this change was long overdue.

Very much appreciated! LGTM!

This revision is now accepted and ready to land.Dec 23 2019, 2:50 PM
NoQ accepted this revision.Dec 23 2019, 3:27 PM

Woohoo tests!~~

clang/unittests/Analysis/CFGBuildResult.h
69

It should be slightly more visually appealing to construct the AST before the callback and pass it into the constructor of the callback.

This revision was automatically updated to reflect the committed changes.