Page MenuHomePhabricator

[analyzer][Dominator] Add post dominator tree builder for the CFG + a debug checker
ClosedPublic

Authored by Szelethus on May 28 2019, 2:03 PM.

Details

Summary

Transform clang::DominatorTree to be able to also calculate post dominators.

  • Tidy up the documentation
  • Make it clang::DominatorTree template class (similarly to how llvm::DominatorTreeBase works), rename it to clang::CFGDominatorTreeImpl
    • Clang's dominator tree is now called clang::CFGDomTree
    • Clang's brand new post dominator tree is called clang::CFGPostDomTree
  • Add a lot of asserts to the dump() function
  • Create a new checker to test the functionality

Diff Detail

Repository
rL LLVM

Event Timeline

Szelethus created this revision.May 28 2019, 2:03 PM
kuhar added inline comments.May 28 2019, 2:16 PM
clang/include/clang/Analysis/Analyses/Dominators.h
39 ↗(On Diff #201758)

Seems outdated?

47 ↗(On Diff #201758)

nit: I think this can name can be somewhat confusing as it's not a base class of CFGDomTree.

49 ↗(On Diff #201758)

Why not have it as a value member? Or a unique_ptr, if pointer semantics are really desired.

111 ↗(On Diff #201758)

Assertions with multiple conditions conjoined are difficult to debug -- perhaps it'd be better to split them and assign to separate booleans for each condition, with descriptive names? This code can be hidden behind an ifdef for debug.

174 ↗(On Diff #201758)

nit: How about having the parent class template called CFGDominatorTreeImpl and these two as CFGDomTree and CFGPostdomTree?

Szelethus updated this revision to Diff 201769.May 28 2019, 2:54 PM
Szelethus edited the summary of this revision. (Show Details)

Fixes according to @kuhar's comments, thanks!

Szelethus marked 5 inline comments as done.May 28 2019, 2:56 PM
Szelethus added inline comments.
clang/include/clang/Analysis/Analyses/Dominators.h
111 ↗(On Diff #201758)

Do you like this better? I prefer running static analysis on large codebases with a Release+Asserts build, and I fear that this check will be lost if I use #ifdef DEBUG.

kuhar added inline comments.May 28 2019, 2:59 PM
clang/include/clang/Analysis/Analyses/Dominators.h
111 ↗(On Diff #201758)

I think it's much more readable now, thanks! Release+Asserts defines NDEBUG and the assertion will work if you put it into an ifdef.

kuhar added inline comments.May 28 2019, 3:02 PM
clang/include/clang/Analysis/Analyses/Dominators.h
111 ↗(On Diff #201758)

*does not define NDEBUG

NoQ accepted this revision.May 28 2019, 6:35 PM

Dunno, looks great :)

Also thx @kuhar!

clang/include/clang/Analysis/Analyses/Dominators.h
194–195 ↗(On Diff #201769)

Is this the intended formatting?

This revision is now accepted and ready to land.May 28 2019, 6:35 PM

Cascade the test file readability improvement from the previous patch.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 3 2019, 4:39 AM