[cfe-dev] Prevent RetainCountChecker from Analyzing Function Bodies when They Have Certain Annotate Attribute

[Alexandre Isoard via cfe-dev]

I would include obj_alloc_* too.

More exactly, any function interacting with reference counters:

$ rc -o -R isl_map::ref
isl_map_copy
isl_map_cow
isl_map_alloc_space
isl_map_free

On Wed, Jul 12, 2017 at 1:42 PM, Malhar Thakkar <cs13b1031 at iith.ac.in>
wrote:

>
>
> On Jul 12, 2017 10:35, "Devin Coughlin" <dcoughlin at apple.com> wrote:
>
>
> On Jul 11, 2017, at 1:36 PM, Devin Coughlin via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>
> On Jul 11, 2017, at 1:12 PM, Malhar Thakkar via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>
>
> On Tue, Jul 11, 2017 at 10:49 PM, Devin Coughlin <devin.coughlin at gmail.com
> > wrote:
>
>>
>> On Jul 11, 2017, at 5:02 AM, Malhar Thakkar via cfe-dev <
>> cfe-dev at lists.llvm.org> wrote:
>>
>>
>>
>> On Tue, Jul 11, 2017 at 5:03 PM, Sven Verdoolaege <skimo-cfe at kotnet.org>
>> wrote:
>>
>>> On Tue, Jul 11, 2017 at 10:33:06AM +0530, Malhar Thakkar wrote:
>>> > Hence, although evalCall() works perfectly for ISL, we may not be able
>>> to
>>> > generalize it for other C codebases.
>>>
>>> I think it's reasonable to assume that frameworks that shield off
>>> free for reference counting, would also shield off malloc
>>> in order to initialize the reference counting.
>>> Of course, this may just be a lack of imagination on my part.
>>> Do you have any examples of frameworks that could use your
>>> annotations where this is not the case?
>>>
>> Well, I haven't come across/thought of any such codebase which doesn't
>> shield off malloc which is why I created a hypothetical test case. Now that
>> you mention it, it does seem reasonable to assume that frameworks would
>> shield off malloc as well.
>>
>>
>> There are some frameworks/idioms that allow a transfer of ownership from
>> a raw malloc’d pointer to a referenced-counted opaque pointer container
>> (this is common in C++ where you use new to allocate and initialize an
>> object). However, I think the ‘trusted implementation’ annotation could be
>> extended to handle this as well. It would require the function that
>> transfers ownership to the reference-counted implementation to be
>> annotated, but I don’t think that is a high burden. That said, I wouldn’t
>> worry about raw malloc’d pointer case for now.
>>
>>
>> Also, keeping MallocChecker aside, there are a lot of other checkers
>> which may create some issues if there are additional side-effects (as Dr.
>> Artem mentioned) in such annotated functions. However, I guess it may be
>> safe to assume that there are no such additional side-effects in such
>> "trusted" functions.
>>
>>
>> I suspect that many of these side effects would have high-level
>> invariants that the analyzer on its own wouldn’t be able to discover (for
>> example, that a set created with a single item in it is not empty) — so
>> custom modeling would be needed in any event, even if the calls were
>> inlined.
>>
>> Devin
>>
>>
>>
>>> skimo
>>>
>>
>> Now, I was experimenting a bit more with evalCall-ing based on
>> annotations and although this works like a charm for functions of the type
>> obj_free() and obj_cow(), it is unable to avoid the problems created by
>> obj_copy(). This is probably because of the lack of a core-foundation
>> annotation which is analogous to isl_keep.
>> Consider the following example.
>>
>>
>> #define __isl_give __attribute__((cf_returns_retained))
>> #define __isl_take __attribute__((cf_consumed))
>> #define __isl_null
>> #define __isl_keep
>> #define NULL 0
>>
>> typedef struct
>> {
>>   int ref;
>> } isl_basic_map;
>>
>> __isl_give isl_basic_map *isl_basic_map_cow(__isl_take isl_basic_map
>> *bmap);
>> __isl_null isl_basic_map *isl_basic_map_free(__isl_take isl_basic_map
>> *bmap);
>>
>> __attribute__((annotate("rc_ownership_trusted_implementation")))
>> __isl_give isl_basic_map *isl_basic_map_copy(__isl_keep isl_basic_map *bmap)
>> {
>>   if (!bmap)
>>     return NULL;
>>
>>   bmap->ref++;
>>   return bmap;
>> }
>>
>> void test_use_after_release_with_trusted_implementation_annotate_attribute(__isl_take
>> isl_basic_map *bmap) {
>>   bmap = isl_basic_map_cow(bmap);
>>   isl_basic_map *temp = isl_basic_map_cow(isl_basic_map_copy(bmap)); *//
>> Here, the analyzer states "Object released".*
>>   isl_basic_map *temp2 = isl_basic_map_cow(bmap); *// Use-after-release
>> for 'bmap' raised here.*
>>   isl_basic_map_free(temp2);
>>   isl_basic_map_free(temp);
>> }
>>
>>
>> I don’t understand what is going on in the above. Can you please simplify
>> the example and explain what is going on and what you expected the analyzer
>> to do? What call is the analyzer complaining about? Is it inlining the
>> call? Please present the simplest example that illustrates the problem.
>>
>
>
> #define __isl_keep
> isl_basic_map *foo(__isl_keep isl_basic_map *bmap);
>
> Consider the above code snippet. Now, I feel that when a parameter has no
> annotation associated to it in some function (foo, in this case), the
> analyzer assumes that the object returned by that function (foo) points to
> the same memory location as that pointed to by the argument passed to it
> (bmap). Please correct me if I'm wrong.
>
>
> The analyzer shouldn’t be doing this when the foo is not inlined. If foo
> is inlined it should only do this if foo() returns its single parameter.
> Is foo() being inlined, or not? If so, what is its body? Is this when
> evalCall is used, or not?
>
>
> I think I understand what you’ve been describing. If you are using the
> evalCall currently in the retain count checker, it is modeling CFRetain(),
> which returns its argument. For your purposes you will not want to return
> the argument but instead conjure a symbol when the called function has the
> trusted annotation. This essentially tells the analyzer that it can’t
> assume anything about the return value. There is code
> in RetainCountChecker::evalCall to do this:
>
>    RetVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, ResultTy,
> C.blockCount());
>
> but it only kicks in when the first argument is unknown. You’ll want to
> extend it to conjure a symbol whenever the evaluated call has the trusted
> annotation and has a return type that isn’t void.
>
> Yes, this is exactly what I needed. I added that functionality and it
> seems to be working now. So, just to make sure everything's working as
> expected, I am currently running the static analyzer again on the ISL
> codebase after adding trusted implementation annotations to obj_free(),
> obj_cow() and obj_copy().
>
> Hope this helps,
>
> Devin
>
>
>


-- 
*Alexandre Isoard*
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170712/5fba49fb/attachment.html>