Hi Jordy,<br><br>I applied most of your patch, except that I adopted 'bindDefault' approach to set the default value, since it does not touch GRExprEngine. Thanks for working on this!<br><br><div class="gmail_quote">
On Sun, May 30, 2010 at 3:46 PM, Jordy Rose <span dir="ltr"><<a href="mailto:jediknil@belkadan.com">jediknil@belkadan.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
Binding a symbolic region whose type is a reference shows up when the<br>
reference is an argument, like so:<br>
<br>
char t3 (char& r) {<br>
r = 'c';<br>
if (r) return r;<br>
return '0';<br>
}<br>
<br>
The reason for the SymbolicRegion section in canHaveDirectBinding(),<br>
though, was originally more about having a way to set default values by<br>
taking advantages of a fact about SymbolicRegions (if you're accessing them<br>
directly, it's either *p or a reference, or an explicit call to Bind()),<br>
not enforcing a rule.<br>
<br>
For looking up super regions' direct bindings, I tried commenting out that<br>
entire section, but it makes the case you mentioned fail<br>
(no-outofbounds.c). Looking at each of the inside IFs:<br>
1. SVN blame says this fixed a crash in misc-ps-region-store-x86_64.m.<br>
2. Same as your example, but with x not yet defined:<br>
int x;<br>
char *p = &x;<br>
return p[0]; // expected-warning {{Undefined}}<br>
3. LazyCompoundVal is LazyCompoundVal...i.e. I don't fully understand what<br>
circumstances they're used in. On the other hand, if I understand<br>
correctly, things with compound values should never have direct bindings<br>
/other/ than LazyCompoundVal.<br>
<br>
It seems like if there's any chance of the feature being useful (even<br>
UnknownVal vs UndefinedVal) this section has to stay. Between<br>
no-outofbounds.c and the commit message about misc-ps-region-store-x86_64.m<br>
crashing; I didn't pry much deeper.<br>
<br>
<br>
As for the calloc() part of the patch, it depends on how default values<br>
should be set from outside RegionStore.cpp, which calloc() needs to be<br>
properly modeled. (And malloc(), for that matter.) I came up with three<br>
options:<br>
1. Add a new BindDefault() method to Store, make it a no-op in BasicStore<br>
and implement it for real in RegionStore.<br>
2. Add a new parameter to SetExtent(). After all, anything with an<br>
explicit extent doesn't get the usual BindArray initialization, and so it's<br>
going to /need/ some kind of initialization. Downside: the rest of the<br>
Extent code isn't related (though the only extents right now are malloc<br>
regions and alloca regions, which could both use this).<br>
3. Define binding to a symbolic region as setting a default value. Because<br>
my fix for PR7218 already changed how lookups worked for arrays, this<br>
didn't seem too far afield, but that's not really a great reason. This<br>
involved the part about changing *p to use ElementRegions ahead of time, so<br>
that the only SymbolicRegions that would make it to Bind() would be the<br>
kind that could have default values (...and references).<br>
<br>
I chose #3, but either of the other two would also work. #2 is actually my<br>
favorite, if we don't mind linking regions and default values conceptually.<br>
<br>
Guess I should have submitted this in two pieces. It's because I was<br>
working on calloc() first that I didn't think of that ahead of time. Let's<br>
get PR7218 nailed down first, then decide how calloc() can set a default<br>
value from outside RegionStore.cpp.<br>
<br>
Sorry...? And thanks for the review, and pointing me in the right<br>
direction.<br>
Jordy<br>
(IRC: jediknil)<br>
<br>
<br>
On Sat, 29 May 2010 14:55:08 +0800, Zhongxing Xu <<a href="mailto:xuzhongxing@gmail.com">xuzhongxing@gmail.com</a>><br>
wrote:<br>
<div><div></div><div class="h5">> I'm thinking about the whole logic below. Does it make sense to try to<br>
get<br>
> the direct binding of the super region of an element region?<br>
><br>
> I can only think of one case:<br>
><br>
> int x = 1;<br>
> char *y = &x;<br>
> y[2];<br>
><br>
> But this case only triggers 'return UnknownVal();' in the last. What<br>
cases<br>
> does the 3 'if' above deal with?<br>
><br>
> RegionStore.cpp:1177<br>
><br>
> // Check if the immediate super region has a direct binding.<br>
> if (const Optional<SVal> &V = getDirectBinding(B, superR)) {<br>
> if (SymbolRef parentSym = V->getAsSymbol()) {<br>
> return ValMgr.getDerivedRegionValueSymbolVal(parentSym, R);<br>
> }<br>
><br>
> if (V->isUnknownOrUndef())<br>
> return *V;<br>
><br>
> // Handle LazyCompoundVals for the immediate super region. Other<br>
cases<br>
> // are handled in 'RetrieveFieldOrElementCommon'.<br>
> if (const nonloc::LazyCompoundVal *LCV =<br>
> dyn_cast<nonloc::LazyCompoundVal>(V)) {<br>
> R = MRMgr.getElementRegionWithSuper(R, LCV->getRegion());<br>
> return RetrieveElement(LCV->getStore(), R);<br>
> }<br>
><br>
> // Other cases: give up.<br>
> return UnknownVal();<br>
> }<br>
</div></div></blockquote></div><br>