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

[Richard]

Hey,

I have a week or so free between projects, and I thought I would have another shot at getting this weak function static analyzer code into a better shape. How about the attached diff? I tried your suggestion of moving the weak function checking into evalCastFromLoc, which works pretty well in fact, not sure what I was doing before. The interface between the constraint manager and the SValBuilder is a little strange I think, open to suggestions here if you have a simpler idea. Otherwise it is a pretty non intrusive patch which passes the attached test. Let me know what you think. 

Richard


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

> 
> On Jan 29, 2013, at 2:05 , Richard <tarka.t.otter at googlemail.com> wrote:
> 
>> 
>> On 29 Jan 2013, at 03:30, Jordan Rose <jordan_rose at apple.com> wrote:
>> 
>>> 
>>> On Jan 27, 2013, at 13:40 , Richard <tarka.t.otter at googlemail.com> wrote:
>>> 
>>>> 
>>>> On 25 Jan 2013, at 18:51, Jordan Rose <jordan_rose at apple.com> wrote:
>>>> 
>>>>> 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.
>>>> 
>>>> This is a nice idea, but I don't think it will work. The problem is that evalCastFromLoc will then expect different return values for the same inputs depending on whether it is being called from evalBinOpLL or evalCast. I gave it a quick go, but it caused many tests to fail for varying reasons. 
>>> 
>>> I'm confused by this. When is a cast of a weak function pointer to bool going to be different from the symbolic value cast to bool? (Or the symbolic value itself, if you type it as 'bool' for now.) And a strong function pointer casted to bool should just be true, always.
>>> 
>> 
>> I believe the problem was trying to match the default behaviour of the switch in evalBinOpLL, which returns true for any MemRegionVal, but evalCast returns UnknownVal in some cases. The tests failing were mostly c++ casting that expected unknown values but were getting trues instead. In other cases checkers were getting null pointers unexpectedly, which caused lots of malloc and retain / release checkers to fail. eg
>> 
>> void testNewArrayNoThrow() {
>>   clang_analyzer_eval(new (1) NoThrow[2]); // expected-warning{{UNKNOWN}}, but got TRUE
>> }
> 
> That's not exactly true -- evalBinOpLL returns true for any non-symbolic MemRegionVal, as should evalCastFromLoc-to-bool.
> 
> I'll see if I can get some time to play around with this and see what sort of difficulties come up.
> 
> (Sorry for drawing out the review process for so long! There are plenty of other things I'm working on right now, but that's probably little comfort to you.)
> 
> Jordan

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130602/82f13ddc/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: weak5.diff
Type: application/octet-stream
Size: 10354 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130602/82f13ddc/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20130602/82f13ddc/attachment-0001.html>