Page MenuHomePhabricator

[analyzer] Don't clean up dead symbols from constraints twice.
ClosedPublic

Authored by NoQ on Nov 12 2019, 4:00 PM.

Details

Summary

A solid 2% analysis speed improvement! Not really - such amounts are barely measurable. Unless?...

But, i mean, everybody knows that dead symbol elimination is the biggest performance bottleneck, so many people tried to improve it, how come nobody noticed this bug before?

Diff Detail

Event Timeline

NoQ created this revision.Nov 12 2019, 4:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2019, 4:00 PM
NoQ marked an inline comment as done.Nov 12 2019, 4:01 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
750–752

Note: the second invocation is here.

NoQ marked an inline comment as done.Nov 12 2019, 4:04 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
763–764

Note: The results of the first invocation are discarded here, as the updated state is getting frankensteined by attaching ExprEngine's environment and store to checker's GDM, while range constraints also reside in the GDM.

NoQ marked an inline comment as done.Nov 12 2019, 4:18 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
763–764

This whole frankensteining process is kinda necessary because SymbolReaper needs to be populated with the data from the Environment and the Store. I.e., they're supposed to issue their markLive() calls, otherwise we wouldn't be able to properly judge whether something is live in checkDeadSymbols; but at the same time the intent is to provide the uncleaned state to the callback, so that the checkers had access to full information about the dying symbol.

I'm not sure whether any checkers actually take advantage of such information, but the intent to provide this information looks valid, so i don't plan to undo this decision.

NoQ edited the summary of this revision. (Show Details)Nov 12 2019, 5:14 PM
xazax.hun accepted this revision.Nov 15 2019, 12:13 PM

Nice catch! I wonder if there is a case where the original code is less wasteful, e.g. the majority of the symbols are dead and a lot of checks splitting the execution. But this sounds very unlikely.

This revision is now accepted and ready to land.Nov 15 2019, 12:13 PM
This revision was automatically updated to reflect the committed changes.