This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] NFC: Replace Taint API with a usual inter-checker communication API?
ClosedPublic

Authored by NoQ on Mar 26 2019, 6:44 PM.

Details

Summary

Given how much effort @boga95 is spending on the taint checker, i decided to undig my effort to remove the whole taint API business from the ProgramState class and instead implement it as our usual, modern inter-checker communication API.

Modern as in "putting all such stuff into the ProgramState class clearly doesn't scale, but we still didn't come up with anything better, so let's just put it in the header and see if a better design suddenly emerges in the future after we do it a few hundred times". In other words, this was the plan that is part of the even-more-long-term plan of completely deprecating the void*-based Generic Data Map in favor of something that the analyzer core could introspect and help checkers keep track of, which would reduce boilerplate and make developing checkers easier.

The patch is rebased on top of the current master, but most likely conflicts with @boga95's latest patches. I'm sorry i didn't do this quickly enough, but better late than never, i suppose. I do not insist on this design, but i believe it looks much cleaner.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Mar 26 2019, 6:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2019, 6:44 PM
NoQ edited reviewers, added: boga95; removed: alexfh.Mar 26 2019, 6:45 PM

(whoops sry nvm typo in reviewer list)

Charusso accepted this revision.Mar 27 2019, 1:05 AM

I like the idea to put connecting stuff together in the same file to create more understandable code. LGTM.

clang/lib/StaticAnalyzer/Checkers/Taint.h
8 ↗(On Diff #192398)

Outdated header-blurb.

81 ↗(On Diff #192398)

We left the ProgramState::dump()' context so what about just dump()`?

This revision is now accepted and ready to land.Mar 27 2019, 1:05 AM

Thanks, it will make my changes more cleaner.

NoQ updated this revision to Diff 192492.Mar 27 2019, 12:05 PM
NoQ marked 2 inline comments as done.

Address comments. Fix printing taint in state dumps.

clang/lib/StaticAnalyzer/Checkers/Taint.h
8 ↗(On Diff #192398)

Whooooooooooops! Thanks!!

81 ↗(On Diff #192398)

I'm doing using namespace taint; everywhere, so that'd make the code look like dump(State) which would look quite ambiguous to a human. Additionally, this function only needs to be called once, so it's not a problem if it's a bit long.

Also, wait, i didn't really call it even once. I.e., i forgot to implement GenericTaintChecker::printState() to print taint (now that it's not state-specific, there must be a checker responsible for this, and i'm casting my vote for GenericTaintChecker out of 1 potential candidates). Let me fix it.

NoQ updated this revision to Diff 192504.Mar 27 2019, 1:12 PM

Add a test for taint dumps.

Szelethus accepted this revision.Mar 28 2019, 3:50 AM

"putting all such stuff into the ProgramState class clearly doesn't scale, but we still didn't come up with anything better, so let's just put it in the header and see if a better design suddenly emerges in the future after we do it a few hundred times"

Now that we have a maintainer who will dedicate a lot of effort into taint analysis, that might just happen :) For the time being though, I agree, this is much cleaner.

It would be great to land this as soon as you're comfortable with it, in order not to block @boga95's work.

Also, you might as well clang-format the new files, since we already messed with git blame.

NoQ updated this revision to Diff 192919.Mar 29 2019, 2:47 PM

Cleaned up formatting a bit.

This revision was automatically updated to reflect the committed changes.
lib/StaticAnalyzer/Core/ProgramState.cpp