This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Add simplistic constant-propagation analysis.
ClosedPublic

Authored by ymandel on Dec 14 2021, 9:20 AM.

Details

Summary

Adds a very simple constant-propagation analysis for demo and testing purposes.

Also, fixes two (small) bugs in the testing framework code.

Diff Detail

Unit TestsFailed

Event Timeline

ymandel created this revision.Dec 14 2021, 9:20 AM
ymandel updated this revision to Diff 394325.Dec 14 2021, 11:20 AM

fix potential null pointer deref.

ymandel updated this revision to Diff 394417.Dec 14 2021, 5:08 PM

Reworks analysis to use bottom, fixes two small bugs in testing framework.

ymandel updated this revision to Diff 394549.Dec 15 2021, 6:24 AM

revised bottom representation and updated tests

ymandel edited the summary of this revision. (Show Details)Dec 15 2021, 6:26 AM
ymandel added reviewers: gribozavr2, sgatev, xazax.hun.
ymandel published this revision for review.Dec 15 2021, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2021, 6:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ymandel updated this revision to Diff 394566.Dec 15 2021, 7:34 AM

Coalesce all CP files into one test file.

ymandel updated this revision to Diff 394568.Dec 15 2021, 7:36 AM

updated file header

sgatev added inline comments.Dec 15 2021, 8:05 AM
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
69 ↗(On Diff #394568)

Should this be a const ref? Same below.

97 ↗(On Diff #394568)

Why not put the definitions below also in the anonymous namespace?

119 ↗(On Diff #394568)

What do you think about inlining this in ConstantPropagationAnalysis::transfer? We don't expect to call it from elsewhere, right?

160 ↗(On Diff #394568)

Maybe call this S to disambiguate from the name of the type?

161 ↗(On Diff #394568)

Should this be a const ref?

180 ↗(On Diff #394568)

Maybe call this HasConstVal if you'd like to keep this short?

197 ↗(On Diff #394568)

Is this necessary?

217 ↗(On Diff #394568)

Capitalize - Code? Or maybe inline this in the function call? Same below.

245 ↗(On Diff #394568)

Maybe add tests for assignment from a function call (e.g. int target = foo();) and assignment from an arithmetic expression (e.g. int target = 1 + 2;)? Both should result in IsUnknown() state, right?

342 ↗(On Diff #394568)

Add internal checks, similar to those in SameAssignmentInBranches?

This demo looks cool. While I know this is a demo, I think it misses some important features, namely invalidating the value when we take the address of a variable or pass it to a function by non-const reference.
I'm fine with not supporting those cases, but I'd love to see a comment about that. Just in case someone wants to use this analysis as a basis for something in production :)

clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
99 ↗(On Diff #394568)

Do we need an actual array? I think this would copy "var" into a global array. Alternatively, using static constexpr char* kVar= "var", we would just refer to the string in the read only memory. (Or maybe both would behave the same because compiler optimizations?)

ymandel updated this revision to Diff 394594.Dec 15 2021, 9:53 AM
ymandel marked an inline comment as done.

Responds to reviewer comments.

ymandel marked 10 inline comments as done.Dec 15 2021, 10:03 AM
ymandel added inline comments.
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
97 ↗(On Diff #394568)

LLVM style is to limit use of anonymous namespaces to class declarations (which have no parallel to static).

99 ↗(On Diff #394568)

Good question. I prefer this style because the array form carries the bounds information with it, unlike the raw pointer form: https://godbolt.org/z/jEP9Wasrh But, I don't know of any meaningful performance implications. FWIW, this is the recommended form for our internal codebase.

119 ↗(On Diff #394568)

inlined the matcher too

180 ↗(On Diff #394568)

I'm fine with the current length. And, a little worried that "const" has its own meaning, which is separate from this one.

217 ↗(On Diff #394568)

capitalized, but prefer to keep it separate for readability. Also, generally good practice since some compliers don't allow these kinds of strings inline in macro calls.

245 ↗(On Diff #394568)

Actually, the eval engine is more powerful than you'd guess -- 1 + 2 evaluates statically. :)

sgatev accepted this revision.Dec 15 2021, 10:31 AM
sgatev added inline comments.
clang/unittests/Analysis/FlowSensitive/SingleVarConstantPropagationTest.cpp
145 ↗(On Diff #394594)

Move this above to replace the other call to getASTContext() or use getASTContext() everywhere?

This revision is now accepted and ready to land.Dec 15 2021, 10:31 AM
ymandel updated this revision to Diff 394617.Dec 15 2021, 11:25 AM
ymandel marked 5 inline comments as done.

address review comments

This revision was landed with ongoing or failed builds.Dec 15 2021, 11:34 AM
This revision was automatically updated to reflect the committed changes.