Page MenuHomePhabricator

[analyzer] DynamicSize: Remove 'getExtent()' from regions
ClosedPublic

Authored by Charusso on Oct 28 2019, 5:43 PM.

Details

Summary

This patch introduces a placeholder for representing the dynamic size of
regions. It also moves the getExtent() method of SubRegions to the
MemRegionManager as getStaticSize().

Diff Detail

Event Timeline

Charusso created this revision.Oct 28 2019, 5:43 PM
Charusso marked 3 inline comments as done.Oct 28 2019, 5:51 PM
Charusso added inline comments.
clang/lib/StaticAnalyzer/Core/RegionStore.cpp
880

That is why the static size information needs to be obtainable by a manager, which seems like a design problem from the very beginning.

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
158

That is the breaking test's code, which is super wonky. I cannot understand what is the rational behind this concept.

clang/test/Analysis/weak-functions.c
10

I have literally copy-pasted the implementation of getExtent(), so I could not figured out why this test has been changed.

NoQ added inline comments.Oct 29 2019, 4:45 PM
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
1253–1255

This looks like a layering violation to me. It's not super important, but i'd rather not have MemRegion depend on SValBuilder.

Can we have getStaticSize() be a method on SValBuilder instead? Or simply a standalone static function in DynamicSize.cpp? 'Cause ideally it shouldn't be called directly.

clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
95–99

As the next obvious step for the next patch, i suggest replacing evalEQ() with some sort of setDynamicSize() here.

clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
1074

And here.

clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
1413

And here.

1547

And here.

clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
176

And here.

clang/lib/StaticAnalyzer/Core/DynamicSize.cpp
29–30

For now the map is always empty, right? Maybe we should remove the map until we actually add a setter.

clang/lib/StaticAnalyzer/Core/MemRegion.cpp
692–693

This code doesn't make much sense; it'll return a pointer size, which has nothing to do with the size of the region.

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
158

Your new code would return a concrete integer here:

case MemRegion::FunctionCodeRegionKind: {
  QualType Ty = cast<TypedRegion>(SR)->getDesugaredLocationType(Ctx);
  return getTypeSize(Ty, Ctx, SVB);
}

Previously it was a symbol.

That said, the original code looks like a super gross hack: they used an extent symbol not because they actually needed an extent, but because they didn't have a better symbol to use :/ I guess you should just keep the extent symbol for now :/

Charusso updated this revision to Diff 227006.Oct 29 2019, 6:12 PM
Charusso marked 12 inline comments as done.
  • Fix.
  • Added a FIXME about the missing symbol of BlockDataRegion, BlockCodeRegion and FunctionCodeRegion.

Thanks for the review! I am not sure why but after your review I always see the most appropriate design immediately.

clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h
1253–1255

Hm, in my game-dev world every manager knows about every manager, so I felt that it needs to work. I like the idea behind the directness and hiding the implementation, but I believe the MemRegionManager should manage its stuff. Also we are lucky, because the SValBuilder is available everywhere with a tiny stress on the API.

clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
95–99

Okay, good idea, thanks! I want to eliminate getSizeInElements as well.

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
158

I see, that was really missing, whoops. Thanks!

NoQ added inline comments.Oct 29 2019, 6:20 PM
clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
158

Let's keep the hack here, not move it to the region manager. I.e., please create SymbolExtent here directly and then separately keep creating it in getStaticSize(). I.e., don't reuse the code when it isn't supposed to behave identically, even if right now it accidentally does behave identically.

Charusso updated this revision to Diff 227013.Oct 29 2019, 6:29 PM
Charusso marked 2 inline comments as done.
  • Unpack the issue-solving about code regions.
clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
158

Ah, yes, it makes sense. In my mind they are connecting even in fixing them.

NoQ accepted this revision.Nov 1 2019, 3:09 PM

Fantastic, let's land this!

This revision is now accepted and ready to land.Nov 1 2019, 3:09 PM
This revision was automatically updated to reflect the committed changes.