[cfe-dev] Weak function pointers (was "SymbolRef and SVal confusion")

[Richard]

Hey,

On 15 Jan 2013, at 19:30, Jordan Rose <jordan_rose at apple.com> wrote:

> 
> On Jan 15, 2013, at 7:21 , Richard <tarka.t.otter at googlemail.com> wrote:
> 
>> Hi Jordan,
>> 
>> On 15 Jan 2013, at 03:44, Jordan Rose <jordan_rose at apple.com> wrote:
>> 
>>> That's not quite going to work -- what if I explicitly spell out my comparison?
>>> 
>>>> if (MyWeakFunction == NULL) {
>>>> }
>>> 
>>> In this case, SimpleConstraintManager won't go through your new assumeLocSymbol.
>>> 
>>> ...although actually, it won't make it there at all, because SimpleSValBuilder::evalBinOpLL folds null comparisons against non-symbolic regions. That's actually easy enough to fix, though, and if you do that this might actually work.
>> 
>> I am not sure I follow you here. Why will this not work? On line 646 of SimpleSValBuilder we have:
>> 
>> if (SymbolRef lSym = lhs.getAsLocSymbol())
>>     return MakeSymIntVal(lSym, op, rInt->getValue(), resultTy);
>> 
>> The metadata symbol will be returned here by getAsLocSymbol() and everything proceeds as expected. Testing this on the following trivial code shows a divide by zero warning for both branches of the IfStmt:
>> 
>> int myFunc() __attribute__((weak_import));
>> int main(int argc, char *argv[])
>> {
>>   if (myFunc == NULL) {
>>     1 / 0;
>>   } else {
>>     1 / 0;
>>   }
>>   return 0;
>> }
> 
> Oops! I missed your changed to SVals.cpp. (This is what I get for reviewing patches by sight only.)
> 
> I am a little concerned about getAsLocSymbol always returning the metadata symbol, but maybe it's okay. I need to go trace through the ramifications of that. My idea was to only produce the metadata symbol when we are sure we are doing a comparison (or a cast to bool, I suppose), since right now there's no way to go from the symbol back to the region (wrapping the symbol in a SymbolicRegion would be incorrect).
> 

Is it not possible to get the region back from the symbol using getRegion()? 

> 
>>> Oh, and metadata symbols also die unless some checker specifically requests to keep them alive. I'm wondering now if the current behavior of SymbolExtent (immutable, lasts as long as the base region, only as path-specific as their region) is, in fact, closer to the desired behavior here than metadata symbols (invalidatable, path-sensitive, only lasts as long as someone is interested). If this is the case, maybe SymbolExtent should be made more generic.
>>> 
>>> (Sorry for giving you a runaround here. I'm not sure what the right thing to do is...I just want to avoid "new symbols" being the solution to every problem, and at the same time make sure that the existing symbols have well-defined semantics.)
>>> 
>> 
>> OK, I was under the impression that a SymbolMetadata would stay alive as long as the MemRegion was, but I see that is incorrect in the docs. I would confess to not exactly being an expert on the Symbol class hierarchy, but it seems to me that SymbolExtent is not the right thing to use here. It has the properties we want, but as you mentioned before, it represents the size of a region. This seems an odd symbol to be using to represent a possible NULL pointer to a function. What about using a SymbolRegionValue here?
> 
> Heh. A bit of history: I added both SymbolExtent and SymbolMetadata. Originally I had hoped they'd be able to use the same kind of symbol, but it turned out that extents being non-invalidatable and metadata being somewhat volatile made it hard to unify the two. If we changed the symbol hierarchy now, we'd rename SymbolExtent -- it'd be something like SymbolRegionMetadata and SymbolRegionContentsMetadata, except those names are ugly.
> 
> The other symbols have single, well-defined purposes, and I'd rather not overload them:
> - SymbolRegionValue represents the contents of a MemRegion, i.e. the value of a parameter or global variable.
> - SymbolDerived represents the contents of a subregion of a SymbolicRegion, e.g. the third field in a struct passed by value.
> - SymbolConjured represents the value of an expression that the analyzer can't evaluate (usually a call expression).
> 
> I/we sometimes cheat and reuse SymbolConjured because it's so flexible, but mostly it's good to stick to these uses. And...
> 
> - SymbolExtent is (currently) a single-purpose symbol used to represent the size (in bytes) of a region whose size is not statically known (i.e. by the compiler).
> - SymbolMetadata is a general-purpose symbol used to represent invalidatable metadata associated with a particular region.
> 
> (I realize all of this should be added to / clarified in our docs.)
> 
> 

SymbolExtent it is then, what do you think of the attached patch? I have changed the QualType of the symbol when it is wrapping a weak FunctionTextRegion to a function pointer type, is this sufficient?

>>> - The initialization of FunctionTextRegion's WeakSym can happen in the constructor initializers rather than the body.
>> 
>> Ja, I wanted to do this, but it seemed like a bit of a chicken and egg thing. To create the symbol requires knowing the FunctionTextRegion in its constructor, so how does the FunctionTextRegion have the symbol in its constructor, without passing it the SymbolManager so it can create the symbol itself, which also seemed a bit odd. Or am I being stupid?
> 
> Ha, I just meant the initialization to NULL. I didn't think through whether it was possible to push the creation of the weak symbol into the constructor.
> 
> Jordan
> 

Then I was being stupid, no easy fix for that unfortunately :|

Richard



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130116/1e50f481/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: weak.diff
Type: application/octet-stream
Size: 6098 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130116/1e50f481/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130116/1e50f481/attachment-0001.html>