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

[Jordan Rose]

I see what you mean about duplicated checks. Here's an idea: put all the checks in evalCastFromLoc under a special case for casting to bool. Then in evalBinOpLL:

if (rInt->isZeroConstant()) {
  if (op == BO_Sub)
    return evalCastFromLoc(lhs, resultTy);
  if (BinaryOperator::isComparisonOp(op))
    return evalBinOpNN(state, op,
                       evalCastFromLoc(lhs, getContext().BoolTy),
                       makeTruthVal(false, getContext().BoolTy));
}

That way you only have the ugly comparison once, and it's in SValBuilder rather than ConstraintManager.

Other comments:

 QualType SymbolExtent::getType() const {
   ASTContext &Ctx = R->getMemRegionManager()->getContext();
+
+  // If this symbol is wrapping a weak function region
+  // then the type should be a function pointer.
+  if (const FunctionTextRegion *FTR = dyn_cast<FunctionTextRegion>(R)) 
+    if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(FTR->getDecl()))
+      if (FD->isWeak())
+        return Ctx.getPointerType(FD->getType());
+
   return Ctx.getSizeType();
 }

This is pretty ugly. I think it's better to just add a QualType field to SymbolExtent and have whoever constructs the object decide the type. It can even default to size_t for now (pending rename/refactoring).


+DefinedSVal SValBuilder::getWeakSymbolForRegion(const FunctionTextRegion *region) {
+  return nonloc::SymbolVal(SymMgr.getExtentSymbol(region));
+}
+

I think this can just return a symbol. Right now it's not really a nonloc, because the symbol type is a pointer. You can make it a SymbolData though to hide the implementation details, or even a fully-generalized SymbolRef.


+void testWeakFunc()
+{
+  clang_analyzer_eval(myFunc == NULL);  // expected-warning{{FALSE}}
+  clang_analyzer_eval(myWeakFunc == NULL);  // expected-warning{{UNKNOWN}}
+  if (myWeakFunc == NULL) {
+    clang_analyzer_eval(myWeakFunc == NULL);  // expected-warning{{TRUE}}
+  }
+}

This is a good start, but you still need more tests: the false branch should have the answer FALSE, and the behavior should be the same if the condition is written 'myWeakFunc' or '!myWeakFunc'.

Getting there...!
Jordan


On Jan 23, 2013, at 11:23 , Richard <tarka.t.otter at googlemail.com> wrote:

> So you mean something like the attached diff? I am not so sure, it makes some things simpler, but I don't like the duplicated messy checks for weak function regions in SimpleSValBuilder and SimpleConstraintManager. What do you reckon? I actually remembered to include a working test this time too.
> 
> <latest.diff>
> 
> On 21 Jan 2013, at 18:48, Jordan Rose <jordan_rose at apple.com> wrote:
> 
>> 
>> On Jan 20, 2013, at 9:00 , Richard <tarka.t.otter at googlemail.com> wrote:
>> 
>>>>> +
>>>>> +  void setWeakSymbol(const SymbolExtent *S) {
>>>>> +    // Only weak functions can have symbols.
>>>>> +    assert(cast<ValueDecl>(FD)->isWeak());
>>>>> +    WeakSym = S;
>>>>> +  }
>>>> 
>>>> MemRegions need to be immutable, so the setWeakSymbol() is not going to work.  Can we just have the symbol associated with the FunctionTextRegion when it gets created?
>>> 
>>> I would like to do this, but I don't see how it is possible easily. The SymbolExtent requires the FunctionTextRegion in its constructor, and I don't see a nice way to get hold of the SymbolManager in the FunctionTextRegions constructor to create it. Any suggestions here?
>> 
>> My new thought on this is that this should just be created lazily by SValBuilder and that it doesn't even need to live on the region. SValBuilder::getWeakSymbolForRegion, or something like that. 
>> 
>> I'll talk to Anna and Ted about what to rename SymbolExtent and SymbolMetadata to properly include this new usage; if you have any opinions, I'd love to hear them!
>> 
>> SymbolExtent:
>> - based on the region itself
>> - does not get invalidated
>> - live as long as the region is live (in theory)
>> 
>> SymbolMetadata:
>> - based on the region contents
>> - is invalidated along with region contents
>> - live as long as the region is live and some checker is interested
>> 
>> Jordan
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130125/2175c515/attachment.html>