This is an archive of the discontinued LLVM Phabricator instance.

[clang][dataflow] Add framework for testing analyses.
ClosedPublic

Authored by ymandel on Dec 8 2021, 6:25 AM.

Diff Detail

Unit TestsFailed

Event Timeline

ymandel created this revision.Dec 8 2021, 6:25 AM
ymandel requested review of this revision.Dec 8 2021, 6:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2021, 6:25 AM
xazax.hun added inline comments.Dec 8 2021, 12:46 PM
clang/unittests/Analysis/FlowSensitive/TestingSupport.h
49

This would also be useful for debugging. I wonder whether utilities not strictly related to testing should be closer to the actual code, so someone wanting to use this for debugging does not need to include the header that is dedicated to the tests.

78

I feel like some of our tests keep recreating lightweight versions of the LibTooling. Not really an action item for this PR, just a rant :) I hope we will have some more centralized stuff at some point.

95

Since this function is actually matching the dataflow results against expectations, I wonder if something like checkDataflow would better describe its function. But feel free to keep the current name.

138

Wouldn't users end up calling the template (not the type erased) version of this function? I wonder if we should mimic how users would interact with the framework in the tests.

ymandel updated this revision to Diff 393218.Dec 9 2021, 10:42 AM

responding to review comments

ymandel marked 2 inline comments as done.Dec 9 2021, 10:51 AM

Thanks for the great comments and the fast response time!

clang/unittests/Analysis/FlowSensitive/TestingSupport.h
49

Agreed. Added a FIXME.

78

No, I agree! I missed this -- there's no good reason for runOnCode to exist. I've removed it and like the result a lot more. :)

95

Agreed.

135

I've inlined buildAnnotationToOutputStateMapping, since it is only intended for use here anyhow and factoring it out didn't save anything because we still need to translate to typed state.

138

I see your point and tried to move in that direction. However, it makes this code a lot messier because of the need to iterate again over each block with transferBlock to recoved statement level state. Since transferBlock is part of the type-erased engine, interacting with it would require mapping all of the state to untyped before using it, which somewhat defeats the whole purpose of using the typed version.

We should consider reporting statement level information from our run...DataflowAnalysis functions, which would make this iteration unnecessary, at which point moving to the typed version would make a lot of sense.

ymandel updated this revision to Diff 393221.Dec 9 2021, 10:54 AM
ymandel marked 2 inline comments as done.

rebasing onto latest version of parent patch

xazax.hun accepted this revision.Dec 9 2021, 10:58 AM

Thanks!

This revision is now accepted and ready to land.Dec 9 2021, 10:58 AM
gribozavr2 accepted this revision.Dec 9 2021, 11:49 AM
ymandel updated this revision to Diff 393460.Dec 10 2021, 6:00 AM

add missing build dependency

This revision was landed with ongoing or failed builds.Dec 10 2021, 6:01 AM
This revision was automatically updated to reflect the committed changes.
ymandel added inline comments.Dec 10 2021, 6:32 AM
clang/unittests/Analysis/FlowSensitive/TestingSupport.h
48–49

This SFINAE guard doesn't work on some platforms, as evidenced by the many buildbot failures after commit.

Any suggestions for

  1. what's wrong
  2. how to configure my local build to replicate the breakage

would be appreciated.

ymandel reopened this revision.Dec 10 2021, 6:57 AM
This revision is now accepted and ready to land.Dec 10 2021, 6:57 AM
ymandel updated this revision to Diff 393477.Dec 10 2021, 7:07 AM

Remove the SFINAE guard and add operator<< for NoopLattice

This revision was landed with ongoing or failed builds.Dec 10 2021, 7:30 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 10 2021, 7:45 AM

Looks like this breaks building on Windows: http://45.33.8.238/win/50614/step_4.txt

Please take a look!

ymandel reopened this revision.Dec 10 2021, 8:52 AM
This revision is now accepted and ready to land.Dec 10 2021, 8:52 AM
ymandel updated this revision to Diff 393513.Dec 10 2021, 8:53 AM

renames the testing namespace to test

cleans up the code the implementation of operator<< for dataflow state.

Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2021, 8:53 AM
This revision was landed with ongoing or failed builds.Dec 11 2021, 5:06 PM
This revision was automatically updated to reflect the committed changes.