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

Sat Jan 12 08:43:14 PST 2013

Hey Jordan,

I agree the solution was a bit messy, it seemed like there were too many places that had to know about testing what kind of SymbolicRegion they had. I prefer your idea of attaching a metadata symbol to the FunctionTextRegion for weak functions, this seems a lot cleaner to me. How about the attached diff? I also added some tests for dispatch_once and CFCopy… method.

Thoughts?

On 12 Jan 2013, at 03:39, Jordan Rose <jordan_rose at apple.com> wrote:

> Hi, Richard. Sorry for the delay in responding.
> 
> Hm. I'm not sure why we need a new symbol type specifically for functions, but I could understand the use for generic weak-linked symbols (unfortunate overloading there). After all, in this case "&foo" may be null:
> 
> extern int foo __attribute__((availability(macosx, 10.4)));
> 
> I'm not sure how we want to represent this in the analyzer, though. We wouldn't want all MemRegions to have associated symbols.
> 
> ...actually, I can think of reasons why we would, at least for all top-level MemRegions. But that's quite a redesign; it's probably not something we're going to do right now.
> 
> In any case, this seems oddly intrusive, with far more places needing to know about SymbolWeakFunction than seems strictly necessary. Having to choose one of two regions available in a single SVal seems very strange, and I'm worried that we won't really be getting this right all the time.
> 
> I'm sorry I'm not able to think through a solid design with you right now; I've got a number of other projects going on. I haven't thought through the ramifications of this yet, but what if you inverted this design and instead gave weak FunctionTextRegions associated metadata symbols with function pointer type? That way it'd be much easier to figure out where FunctionTextRegions need to opt-in to SymbolicRegion-like behavior, and at worst we'd handle these functions the same way we did before. ...Again, I haven't really thought it all the way through, but what do you think?)
> 
> By the way, please make sure your code conforms to the LLVM coding conventions. In particular, we capitalize local variable names, indent with two spaces, attach & and * to the variable name rather than the type name, and keep all lines within 80 columns.
> 
> As for your test cases, I'd personally suggest not using clang_analyzer_eval as a weak function—its only purpose is to be interpreted by the analyzer. malloc() tests checkers evaluating calls. Two other things that might be worth borrowing are dispatch_once, which is implemented with a synthesized body, and some made up CoreFoundation function like CFCopyFoo, which the analyzer handles with a post-call callback to produce a leak warning.
> 
> Jordan
> 
> 
> On Jan 7, 2013, at 10:22 , Richard <tarka.t.otter at googlemail.com> wrote:
> 
>> Hey Jordan,
>> 
>> How about something like the attached diff? I have included a test case that just stole some of the other tests on C functions and redeclares them as weak and runs the tests again. I did also try making all function pointers weak and running the test cases, which also passes. A couple of test cases needed modifying to return the underlying FunctionTextRegion instead of the SymbolicRegion for weak function pointers, which seems a bit messy, don't know if you have any ideas about a nicer solution to this. Maybe getAsRegion() should always return the FunctionTextRegion if it is wrapping a SymbolWeakFunction?
>> 
>> Richard.
>> 
>> <WeakFuncs.diff>
>> <weak-functions.c>
>> 
>> On 5 Jan 2013, at 03:30, Jordan Rose <jordan_rose at apple.com> wrote:
>> 
>>> VisitCast handles the "decay" in the AST from a raw function name to a function pointer; all C function calls are actually calls to function pointers according to the standard. But the actual code that figures out the function to call is in CallEventManager::getSimpleCall, which...huh, doesn't actually look at the callee's SVal if it's known at compile time. Which means only calls through weak function pointers would lose out. I would actually be okay with this since these are (a) rare, and (b) probably not calls we do much special processing for anyway.
>>> 
>>> If you want to try hacking this in, I'd suggest using a conjured symbol with no Expr and no block count (so it's the same all across the program) and the appropriate pointer-to-function type:
>>> 
>>> QualType Ty = Ctx.getPointerType(FD->getType());
>>> SVB.conjureSymbol(/*Stmt=*/0, /*LCtx=*/0, Ty, /*VisitCount=*/0, /*Tag=*/FD);
>>> 
>>> And then come up with a bunch of test cases and make sure that if you, say, define "malloc" as weak that we still treat it like "malloc". If everything works, send it back and I'll commit it to SVN.
>>> 
>>> Thanks for working on this!
>>> Jordan
>>> 
>>> 
>>> On Jan 3, 2013, at 13:15 , Richard <tarka.t.otter at googlemail.com> wrote:
>>> 
>>>> Hey Jordan,
>>>> 
>>>> I realise SymbolExtent is the wrong symbol class to use, it was just a quick hack to see how much more work was involved in getting the analyser to assume false on function decls. Not very much it turned out. I guess a new SymExpr subclass is needed. 
>>>> 
>>>> The bit I am not clear on is where the analyser calls a function, where I would need to add code to handle this new symbol type. Apologies if this is a stupid question, I had a dig through ExprEngine, but did not find what I was looking for. Is it VisitCast?
>>>> 
>>>> Ta.
>>>> 
>>>> On 3 Jan 2013, at 20:22, Jordan Rose <jordan_rose at apple.com> wrote:
>>>> 
>>>>> SymbolExtent isn't really meant for this; it's supposed to represent the metadata of how large an allocation is in memory. Doing this is basically like changing "return func" to "return sizeof(*func)", except that functions don't really have valid sizes anyway. You really can't put an extent symbol (type size_t) into a loc::MemRegionVal (some kind of pointer-ish thing).
>>>>> 
>>>>> In practice, this lets you do the null test, but won't actually let the analyzer call the function, which is no good.
>>>>> 
>>>>> I don't have any other immediate insights to offer. We just don't have values that can represent either null or a specific function at this time. You might be able to fake it for now by adding a pre-visit check for CastExprs of type CK_FunctionToPointerDecay, and eagerly splitting the path whenever someone references a weak function.
>>>>> 
>>>>> Jordan
>>>>> 
>>>>> 
>>>>> On Jan 3, 2013, at 10:03 , Richard <tarka.t.otter at googlemail.com> wrote:
>>>>> 
>>>>>> I had a quick attempt at this, by creating a SymbolExtent of a weak function decl code region and creating a SymbolicRegion with that. This actually fixes the checker I was writing, which is nice. I am not sure if I understand fully the implications of doing this however. Where does the SymbolicRegion need to be constrained back to a FunctionTextRegion?
>>>>>> 
>>>>>> Index: Core/SValBuilder.cpp
>>>>>> ===================================================================
>>>>>> --- Core/SValBuilder.cpp	(revision 171384)
>>>>>> +++ Core/SValBuilder.cpp	(working copy)
>>>>>> @@ -190,7 +190,13 @@
>>>>>>  }
>>>>>>  
>>>>>>  DefinedSVal SValBuilder::getFunctionPointer(const FunctionDecl *func) {
>>>>>> -  return loc::MemRegionVal(MemMgr.getFunctionTextRegion(func));
>>>>>> +  const FunctionTextRegion *Region = MemMgr.getFunctionTextRegion(func);
>>>>>> +  if (func->isWeak()) {
>>>>>> +    const SymbolExtent *Sym = SymMgr.getExtentSymbol(Region);
>>>>>> +    return loc::MemRegionVal(MemMgr.getSymbolicRegion(Sym));
>>>>>> +  }
>>>>>> +    
>>>>>> +  return loc::MemRegionVal(Region);
>>>>>>  }
>>>>>>  
>>>>>>  DefinedSVal SValBuilder::getBlockPointer(const BlockDecl *block,
>>>>>> 
>>>>>> On 20 Dec 2012, at 19:31, Ted Kremenek <kremenek at apple.com> wrote:
>>>>>> 
>>>>>>> On Dec 20, 2012, at 10:14 AM, Jordan Rose <jordan_rose at apple.com> wrote:
>>>>>>> 
>>>>>>>> The problem is that functions are represented by FunctionTextRegions. As you noticed, our design is that only SymbolicRegions can represent NULL—all other regions are known to have an address. However, this is not true for weak symbols (functions or otherwise). In order to get this right, we probably need to enhance the analyzer to treat weak extern symbols like references, and then automatically dereference them upon use.
>>>>>>> 
>>>>>>> I don't think the "references" analogy is quite right.  Functions are already modeled in the AST using function pointers, and they are dereferenced during a function call.  We could possibly model weak-linked functions using SymbolicRegions, that are then later constrained to alias a specific FunctionTextRegion.  Aliasing is something we need to handle better anyway, and I think this would nicely fit into that model.
>>>>>> 
>>>>> 
>>> 
>> 
> 

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