Hi,
I have just been looking into using the clang analyzer retain counter to
check the use of a reference counting library we use as part of the LLVM
Polly project. This seems to work surprisingly well for now and most of
the basic (and not so basic) errors are correctly found. However, it
seems that function arguments are never considered as possibly leaking.
Here a small example:
struct obj;
typedef struct obj obj;
#define ___give __attribute__((cf_returns_retained))
#define ___take __attribute__((cf_consumed))
#define ___take
__give obj *alloc();
__give obj *obj_copy(___keep obj *o);
void obj_free(___take obj *o);
void error_reported() {
obj *o = alloc();
}
void leak_ignored(___take obj *o) {
}
For the function error_reported, the memory leak is correctly warned
about. However, for leak_ignored, no warning is issued. Is this behavior
correct or should the checker report a memory leak here as well?
For my use case, I would like that all function arguments passed with
__take (consumed) are required to be freed in the function (or passed
back as return value), whereas arguments passed as __keep (not consumed)
would need to be copied before they can be used as return value or
passed to any __isl_take function.
My feeling is that I can add this functionality easily by correctly
initializing the state of function arguments. However, as a newcomer to
the clang static analyzer I am not sure where to do this. Is
checkBeginFunction the right location (I fail to even find a way to
obtain the function we currently work on here) or are there more
specialized callbacks that are called when each of the functions
arguments is evaluated?
I probably need to dig deeper into this to make this happen, but as this
is likely a pretty trivial problem for our static analysis experts, I
wanted to check if anybody has some input or hints that might help me
here?
Best,
Tobias
Tobias,
Hi,
I have just been looking into using the clang analyzer retain counter to
check the use of a reference counting library we use as part of the LLVM
Polly project. This seems to work surprisingly well for now and most of
the basic (and not so basic) errors are correctly found. However, it
seems that function arguments are never considered as possibly leaking.
Here a small example:
struct obj;
typedef struct obj obj;
#define ___give __attribute__((cf_returns_retained))
#define ___take __attribute__((cf_consumed))
#define ___take
__give obj *alloc();
__give obj *obj_copy(___keep obj *o);
void obj_free(___take obj *o);
void error_reported() {
obj *o = alloc();
}
void leak_ignored(___take obj *o) {
}
For the function error_reported, the memory leak is correctly warned
about. However, for leak_ignored, no warning is issued. Is this behavior
correct or should the checker report a memory leak here as well?
This behavior is not correct — ideally it would report a memory leak. My understanding is that this functionality just hasn't been added to the RetainCountChecker.
For my use case, I would like that all function arguments passed with
__take (consumed) are required to be freed in the function (or passed
back as return value), whereas arguments passed as __keep (not consumed)
would need to be copied before they can be used as return value or
passed to any __isl_take function.
My feeling is that I can add this functionality easily by correctly
initializing the state of function arguments. However, as a newcomer to
the clang static analyzer I am not sure where to do this. Is
checkBeginFunction the right location (I fail to even find a way to
obtain the function we currently work on here) or are there more
specialized callbacks that are called when each of the functions
arguments is evaluated?
I probably need to dig deeper into this to make this happen, but as this
is likely a pretty trivial problem for our static analysis experts, I
wanted to check if anybody has some input or hints that might help me
here?
There a couple of things to note:
1) The RetainCountChecker (which is the checker that understands the cf_consumed and cf_returns_retained attributes) was designed for checking a reference counting scheme that relies heavily on communicating ownership via the names of functions. The checker uses these naming rules to determine ownership convention. For example, if a function contains the word ‘Copy’ or ‘Create’ it is assumed to act like cf_returns_retained. Typically the attributes are only used when a function violates the naming conventions. There is more information about these naming conventions in the CoreFoundation memory management guide <https://developer.apple.com/library/mac/documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html>\.
2) As for where to start: I would take a look at the logic in void RetainCountChecker::checkSummary() that deals with the return value of a function call and apply it to parameters. You are right that checkBeginFunction() is a good place for this. You can take a look at void CXXSelfAssignmentChecker::checkBeginFunction(CheckerContext &C) to see how another checker gets at the parameter values. Note that it is really important that this only be done when the function is at the top level — we don’t check retain counts for inlined functions.
3) Extending the RetainCountChecker to properly check parameters is something we’re definitely interested in — but you'll have to be a bit careful here because the checker is already widely used. It would be good to keep this extra checking behind an analyzer-config flag at first and off by default until it is fully evaluated.
Hope this helps!
Devin
Tobias,
Devin, this was fast. Thank you!
>
> Hi,
>
> I have just been looking into using the clang analyzer retain counter to
> check the use of a reference counting library we use as part of the LLVM
> Polly project. This seems to work surprisingly well for now and most of
> the basic (and not so basic) errors are correctly found. However, it
> seems that function arguments are never considered as possibly leaking.
> Here a small example:
>
> struct obj;
> typedef struct obj obj;
>
> #define ___give __attribute__((cf_returns_retained))
> #define ___take __attribute__((cf_consumed))
> #define ___take
>
> __give obj *alloc();
> __give obj *obj_copy(___keep obj *o);
> void obj_free(___take obj *o);
>
> void error_reported() {
> obj *o = alloc();
> }
>
> void leak_ignored(___take obj *o) {
> }
>
> For the function error_reported, the memory leak is correctly warned
> about. However, for leak_ignored, no warning is issued. Is this behavior
> correct or should the checker report a memory leak here as well?
This behavior is not correct — ideally it would report a memory leak. My
understanding is that this functionality just hasn't been added to the
RetainCountChecker.
Thanks for confirming.
> For my use case, I would like that all function arguments passed with
> __take (consumed) are required to be freed in the function (or passed
> back as return value), whereas arguments passed as __keep (not consumed)
> would need to be copied before they can be used as return value or
> passed to any __isl_take function.
>
> My feeling is that I can add this functionality easily by correctly
> initializing the state of function arguments. However, as a newcomer to
> the clang static analyzer I am not sure where to do this. Is
> checkBeginFunction the right location (I fail to even find a way to
> obtain the function we currently work on here) or are there more
> specialized callbacks that are called when each of the functions
> arguments is evaluated?
>
> I probably need to dig deeper into this to make this happen, but as this
> is likely a pretty trivial problem for our static analysis experts, I
> wanted to check if anybody has some input or hints that might help me
> here?
There a couple of things to note:
1) The RetainCountChecker (which is the checker that understands the
cf_consumed and cf_returns_retained attributes) was designed for checking
a reference counting scheme that relies heavily on communicating
ownership via the names of functions. The checker uses these naming rules
to determine ownership convention. For example, if a function contains
the word ‘Copy’ or ‘Create’ it is assumed to act like
cf_returns_retained. Typically the attributes are only used when a
function violates the naming conventions. There is more information about
these naming conventions in the CoreFoundation memory management guide
<https://developer.apple.com/library/mac/documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html>\.
2) As for where to start: I would take a look at the logic in void
RetainCountChecker::checkSummary() that deals with the return value of a
function call and apply it to parameters. You are right that
checkBeginFunction() is a good place for this. You can take a look at
void CXXSelfAssignmentChecker::checkBeginFunction(CheckerContext &C) to
see how another checker gets at the parameter values. Note that it is
really important that this only be done when the function is at the top
level — we don’t check retain counts for inlined functions.
Great. I could make it work for my uses cases (see patch for reference).
3) Extending the RetainCountChecker to properly check parameters is
something we’re definitely interested in — but you'll have to be a bit
careful here because the checker is already widely used. It would be good
to keep this extra checking behind an analyzer-config flag at first and
off by default until it is fully evaluated.
Not sure I know how to use the analyzer config flags yet. My patch
currently also still causes a segfault on Analysis/retain-release.m and
changes a test case, where I am not sure it should.
I probably need some time to get this tested and get more confidence in
what it is doing. I will then put it up for review and we can see how to
add an analyzer-config flag.
Best,
Tobias
0001-Support-parameter-checking-in-Retaincounter.patch (3.73 KB)
And here a slightly more cleaned up version of my patch, just for
reference.
Best,
Tobias
0001-RetainCountChecker-Also-check-function-parameters-fo.patch (8.01 KB)