Page MenuHomePhabricator

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

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



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.

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

158 ↗(On Diff #226795)

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

10 ↗(On Diff #226795)

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

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.


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


And here.


And here.


And here.


And here.


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


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

158 ↗(On Diff #226795)

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.


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.


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

158 ↗(On Diff #226795)

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

NoQ added inline comments.Oct 29 2019, 6:20 PM
158 ↗(On Diff #226795)

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.
158 ↗(On Diff #226795)

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.