Page MenuHomePhabricator

[Analyzer] Refactor begin and end symbol creation
ClosedPublic

Authored by baloghadamsoftware on Apr 25 2019, 9:11 AM.

Details

Summary

This patch refactors begin and end symbol creation by moving symbol conjuration into the create... functions. This way the functions' responsibilities are clearer and makes possible to add more functions handling these symbols (e.g. functions for handling the container's size) without code multiplication.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ added a comment.Apr 25 2019, 2:15 PM

Aha, yup, thx!

Do you plan to centralize other code paths on which you conjure symbols?

'Cause i still believe that given that we now support C++ (our prvalues are now properly materialized) (most of the time), you should be able to remove support for symbol-like iterators, and then replace your position symbols with SymbolMetadata.

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
1929–1930 ↗(On Diff #196649)

This is a bit more auto than allowed by our coding standards (it pretty much disallows auto unless it's some sort of dyn_cast (getAs) or an iterator.

2247 ↗(On Diff #196649)

This looks like a new feature. Is it testable?

In D61136#1479454, @NoQ wrote:

Aha, yup, thx!

Do you plan to centralize other code paths on which you conjure symbols?

Only in a small rework of handleAssign() because that is a place where we conjure a symbol for a new end(). I only plan to centralize conjuration of begin() and end() symbols of containers.

'Cause i still believe that given that we now support C++ (our prvalues are now properly materialized) (most of the time), you should be able to remove support for symbol-like iterators, and then replace your position symbols with SymbolMetadata.

Abstract iterator positions are represented by symbolic expressions because we must be able to do simple calculations with them. This has nothing to do with iterator objects represented as symbols. Or what do you mean exactly by "replacing position symbols with SymbolMetadata"?

baloghadamsoftware marked 2 inline comments as done.Apr 26 2019, 12:43 AM
baloghadamsoftware added inline comments.
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
1929–1930 ↗(On Diff #196649)

I can add the type, of course. However, until now I understood and it is also in the coding standard that "other places where the type is already obvious from the context". For me it is obvious that conjureSymbol() returns a SymbolRef. Even more obvious is the getSymbolManager() returns a SymbolManager.

2247 ↗(On Diff #196649)

I am not sure I can test it alone. Maybe I should leave it for now and add it in another patch together with modelling of empty() or size(). Then I should also rename this patch which remains pure refactoring.

whisperity edited the summary of this revision. (Show Details)Apr 26 2019, 3:07 AM
NoQ added a comment.Apr 29 2019, 3:49 PM

Abstract iterator positions are represented by symbolic expressions because we must be able to do simple calculations with them. This has nothing to do with iterator objects represented as symbols. Or what do you mean exactly by "replacing position symbols with SymbolMetadata"?

I mean, use SymbolMetadata wherever you use SymbolConjured. Because metadata is attached to objects (i.e., it takes a region as part of its identity), it's problematic to use with iterators represented as symbols.

Though i'd also consider the opposite direction which seems to be even more promising: re-do SymbolMetadata so that it took a symbol instead of a region as part of its identity, and then attach "identity symbols" to all C++ objects, so that we didn't ever have to track iterators by their regions, but instead we could make IteratorChecker operate more like SimpleStreamChecker. This still requires re-doing all the conjuring, so keeping all the conjuring in one place is still a good idea.

lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
1929–1930 ↗(On Diff #196649)

Here is a discussion on this matter that i had recently. It was hard enough to convince people that the code below follows the coding standards:

auto DV = V.getAs<DefinedOrUnknownSVal>();

Also, conjuredSymbol() in fact returns a const SymbolConjured *, which is a more specialized type. This probably deserves at least a const auto *.

2247 ↗(On Diff #196649)

Yup, i think it's good to keep NFC commits and features apart.

Szelethus requested changes to this revision.Apr 30 2019, 2:46 AM
Szelethus added inline comments.
lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
1929–1930 ↗(On Diff #196649)

+1, I strongly disagree with the overuse of auto. I think project-wide things like the one you mentioned (SVal::getAs) are fine, because after googleing it once of twice it genuinely makes the code far more readable. However, where the scope of the type (like in many cases in this file) is very small, line breaks aren't a concern, or the function just simply isn't widely used (and conjureSymbol really isn't) we shouldn't use auto.

In general, I'd be just far more conservative: whenever in doubt, just don't use it. Whenever it's anything but super-super obvious, don't use it.

This revision now requires changes to proceed.Apr 30 2019, 2:46 AM
baloghadamsoftware retitled this revision from [Analyzer] IteratorChecker - Ensure end()>=begin() and refactor begin and end symbol creation to [Analyzer] Refactor begin and end symbol creation.
baloghadamsoftware edited the summary of this revision. (Show Details)

SymbolConjured * written, feature removed, just refactoring.

NoQ accepted this revision.May 13 2019, 3:06 PM

Thx!

Szelethus accepted this revision.May 13 2019, 3:36 PM
This revision is now accepted and ready to land.May 13 2019, 3:36 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2019, 4:01 AM