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

[Malhar Thakkar via cfe-dev]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170712/7b74ea66/attachment.html>