This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] NFC: MallocChecker: Avoid redundant transitions.
ClosedPublic

Authored by NoQ on Nov 1 2018, 5:04 PM.

Details

Summary

In its checkDeadSymbols callback, when no pointers are tracked and the state is not updated, MallocChecker would update the state from "the map of symbols is unset" to "the map of symbols is set to an empty map", which generates a redundant exploded node. Now that zombie symbols are fixed in D18860, we would call checkDeadSymbols() with no dead symbols more often. Avoid the unnecessary transition here.

Diff Detail

Event Timeline

NoQ created this revision.Nov 1 2018, 5:04 PM

Please reupload with full context.

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2369

We are acquiring an object of type ImmutableMap, modify it, and put it back into state? Can't we just modify it through ProgramState's interface directly?

lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2369

state is immutable, I don't think we can put anything into it.
We also can't modify ImmutableMap because, well, it's immutable.

Szelethus added inline comments.Nov 5 2018, 10:58 AM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2369

Poor choice of words, I admit. What I actually meant is that maybe it would be more straighforward if we didnt create a local varable here. But I'm fine with being in the wrong :)

NoQ updated this revision to Diff 172629.Nov 5 2018, 11:53 AM

Re-upload with context. Whoops.

NoQ retitled this revision from [analyzer] MallocChecker: Avoid redundant transitions. to [analyzer] NFC: MallocChecker: Avoid redundant transitions..Nov 5 2018, 11:56 AM
NoQ added inline comments.
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2369

Manipulating maps directly is slightly cheaper because for every operation you only create a new map but not a new state. I have no indication that this optimization is worth the effort, but i also have no indication that pessimizing it back is worth the effort.

george.karpenkov accepted this revision.Nov 5 2018, 12:00 PM
This revision is now accepted and ready to land.Nov 5 2018, 12:00 PM
Szelethus added inline comments.Nov 5 2018, 1:22 PM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2369

Okay, thanks :)

Szelethus added inline comments.Nov 22 2018, 9:00 AM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2383–2384

Hmmm, I guess we return here because if RegionState is unchanged, so should be ReallocPairs and FreeReturnValue, correct?

NoQ added inline comments.Nov 29 2018, 7:44 PM
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2383–2384

Yeah, kinda. Deserves a comment, i guess.

NoQ marked an inline comment as done.Nov 29 2018, 7:54 PM
NoQ added inline comments.
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
2383–2384

Or even an assertion.

This revision was automatically updated to reflect the committed changes.