[cfe-commits] [PATCH] Fix for PR7218, and analyzer support for calloc()

[Jordy Rose]

Binding a symbolic region whose type is a reference shows up when the
reference is an argument, like so:

char t3 (char& r) {
  r = 'c';
  if (r) return r;
  return '0';
}

The reason for the SymbolicRegion section in canHaveDirectBinding(),
though, was originally more about having a way to set default values by
taking advantages of a fact about SymbolicRegions (if you're accessing them
directly, it's either *p or a reference, or an explicit call to Bind()),
not enforcing a rule.

For looking up super regions' direct bindings, I tried commenting out that
entire section, but it makes the case you mentioned fail
(no-outofbounds.c). Looking at each of the inside IFs:
1. SVN blame says this fixed a crash in misc-ps-region-store-x86_64.m.
2. Same as your example, but with x not yet defined:
  int x;
  char *p = &x;
  return p[0]; // expected-warning {{Undefined}}
3. LazyCompoundVal is LazyCompoundVal...i.e. I don't fully understand what
circumstances they're used in. On the other hand, if I understand
correctly, things with compound values should never have direct bindings
/other/ than LazyCompoundVal.

It seems like if there's any chance of the feature being useful (even
UnknownVal vs UndefinedVal) this section has to stay. Between
no-outofbounds.c and the commit message about misc-ps-region-store-x86_64.m
crashing; I didn't pry much deeper.


As for the calloc() part of the patch, it depends on how default values
should be set from outside RegionStore.cpp, which calloc() needs to be
properly modeled. (And malloc(), for that matter.) I came up with three
options:
1. Add a new BindDefault() method to Store, make it a no-op in BasicStore
and implement it for real in RegionStore.
2. Add a new parameter to SetExtent(). After all, anything with an
explicit extent doesn't get the usual BindArray initialization, and so it's
going to /need/ some kind of initialization. Downside: the rest of the
Extent code isn't related (though the only extents right now are malloc
regions and alloca regions, which could both use this).
3. Define binding to a symbolic region as setting a default value. Because
my fix for PR7218 already changed how lookups worked for arrays, this
didn't seem too far afield, but that's not really a great reason. This
involved the part about changing *p to use ElementRegions ahead of time, so
that the only SymbolicRegions that would make it to Bind() would be the
kind that could have default values (...and references).

I chose #3, but either of the other two would also work. #2 is actually my
favorite, if we don't mind linking regions and default values conceptually.

Guess I should have submitted this in two pieces. It's because I was
working on calloc() first that I didn't think of that ahead of time. Let's
get PR7218 nailed down first, then decide how calloc() can set a default
value from outside RegionStore.cpp.

Sorry...? And thanks for the review, and pointing me in the right
direction.
Jordy
(IRC: jediknil)


On Sat, 29 May 2010 14:55:08 +0800, Zhongxing Xu <xuzhongxing at gmail.com>
wrote:
> I'm thinking about the whole logic below. Does it make sense to try to
get
> the direct binding of the super region of an element region?
> 
> I can only think of one case:
> 
> int x = 1;
> char *y = &x;
> y[2];
> 
> But this case only triggers 'return UnknownVal();' in the last. What
cases
> does the 3 'if' above deal with?
> 
> RegionStore.cpp:1177
> 
>   // Check if the immediate super region has a direct binding.
>   if (const Optional<SVal> &V = getDirectBinding(B, superR)) {
>     if (SymbolRef parentSym = V->getAsSymbol()) {
>       return ValMgr.getDerivedRegionValueSymbolVal(parentSym, R);
>     }
> 
>     if (V->isUnknownOrUndef())
>       return *V;
> 
>     // Handle LazyCompoundVals for the immediate super region.  Other
cases
>     // are handled in 'RetrieveFieldOrElementCommon'.
>     if (const nonloc::LazyCompoundVal *LCV =
>         dyn_cast<nonloc::LazyCompoundVal>(V)) {
>       R = MRMgr.getElementRegionWithSuper(R, LCV->getRegion());
>       return RetrieveElement(LCV->getStore(), R);
>     }
> 
>     // Other cases: give up.
>     return UnknownVal();
>   }