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

[Malhar Thakkar via cfe-dev]

On Sat, Jul 15, 2017 at 4:39 AM, Devin Coughlin <dcoughlin at apple.com> wrote:

>
> On Jul 14, 2017, at 12:31 PM, Malhar Thakkar <cs13b1031 at iith.ac.in> wrote:
>
> evalCall-ing a function based on annotations worked for the most part
> except for the following scenario.
>
> If the definition of the annotated function is *after *it is called
> somewhere in the code, the RetainCountChecker is unable to "see" the
> annotation on this function (I checked this by printing debug messages in
> evalCall to see if the RetainCountChecker is able to find the annotation).
>
> Consider the following examples.
>
> *// Definition of annotated function after it is called.*
>
> #define NULL 0typedef struct
> {
> 	int ref;
> } isl_basic_map;
> void free(void *);
> __isl_give isl_basic_map *foo(__isl_take isl_basic_map *bmap);
> isl_basic_map *isl_basic_map_free(__isl_take isl_basic_map *bmap);
>
> __isl_give isl_basic_map *bar(__isl_take isl_basic_map *bmap) {
> 	bmap = foo(bmap);
> 	return isl_basic_map_free(bmap); *// Leak warning for 'bmap' raised here.*
> }
>
> __attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__isl_take isl_basic_map *bmap)
> {
> 	if (!bmap)
> 		return NULL;
>
> 	if (--bmap->ref > 0)
> 		return NULL;
>
> 	free(bmap);
> 	return NULL;
> }
>
>
> *// Definition of annotated function before it is called.*
>
> #define NULL 0typedef struct
> {
> 	int ref;
> } isl_basic_map;
> void free(void *);
> __isl_give isl_basic_map *foo(__isl_take isl_basic_map *bmap);
> isl_basic_map *isl_basic_map_free(__isl_take isl_basic_map *bmap);
>
> __attribute__((annotate("rc_ownership_trusted_implementation"))) isl_basic_map *isl_basic_map_free(__isl_take isl_basic_map *bmap)
> {
> 	if (!bmap)
> 		return NULL;
>
> 	if (--bmap->ref > 0)
> 		return NULL;
>
> 	free(bmap);
> 	return NULL;
> }
>
> __isl_give isl_basic_map *bar(__isl_take isl_basic_map *bmap) {
> 	bmap = foo(bmap);
> 	return isl_basic_map_free(bmap); *// No leak warning for 'bmap' raised here.*
> }
>
> It would really help me if someone could point out why the
> RetainCountChecker behaves this way.
>
>
> I don’t know. One thing to note is that unlike most annotations, this one
> seems like it belongs on the definition of the function and not its
> interface, so you probably want to use getDefinition() to get the FuncDecl
> corresponding to the definition of the function body.
>

Yes, this works. I have uploaded a patch <https://reviews.llvm.org/D34937>
on Phabricator for the same.

>
> Also, I have a few queries directed to Dr. Alexandre and Dr. Sven.
> I applied these trusted annotations to obj_free(), obj_cow() and
> obj_copy() as they have a pattern (__isl.*free, __isl.*cow and
> __isl.*copy).
>
> Do, functions of the type obj_alloc_* have the same pattern if at all they
> do? Also, are there any other functions which require such annotations?
>
>
> It would be helpful if you explained what the obj_alloc_* functions do. Do
> they malloc memory directly and return as __isl_give? Do they manipulate
> reference counts directly?
>

Generally, obj_alloc_* functions initialize all the fields (including
initializing the reference counter to 1) of that particular isl object
(obj) and then return obj as __isl_give.
However, there are certain scenarios in which the objects passed to
obj_alloc_* functions are *reference counted objects* and are annotated
with __isl_take in which case leak warnings are raised.

Example

*// In the below example, the reference counts of expr1 and expr2 are
not decremented.*

*// Here, expr1 and expr2 are reference counted objects.*

*// Analysis starts from '*isl_ast_node_for_get_cond'

__attribute__((annotate("rc_ownership_trusted_implementation")))
__isl_give isl_ast_expr *isl_ast_expr_copy(__isl_keep isl_ast_expr
*expr)
{
	if (!expr)
		return NULL;

	expr->ref++;
	return expr;
}

__isl_give isl_ast_expr *isl_ast_expr_alloc_binary(enum isl_ast_op_type type,
	__isl_take isl_ast_expr *expr1, __isl_take isl_ast_expr *expr2)
{
	isl_ctx *ctx;
	isl_ast_expr *expr = NULL;

	if (!expr1 || !expr2)
		goto error;

	ctx = isl_ast_expr_get_ctx(expr1);
	expr = isl_ast_expr_alloc_op(ctx, type, 2);
	if (!expr)
		goto error;

	expr->u.op.args[0] = expr1;
	expr->u.op.args[1] = expr2; *// Leak warning raised for expr1*

	return expr; *// Leak warning raised for expr2*error:
	isl_ast_expr_free(expr1);
	isl_ast_expr_free(expr2);
	return NULL;
}

__isl_give isl_ast_expr *isl_ast_node_for_get_cond(
	__isl_keep isl_ast_node *node)
{
	if (!node)
		return NULL;
	if (node->type != isl_ast_node_for)
		isl_die(isl_ast_node_get_ctx(node), isl_error_invalid,
			"not a for node", return NULL);
	if (!node->u.f.degenerate)
		return isl_ast_expr_copy(node->u.f.cond);

	return *isl_ast_expr_alloc_binary*(isl_ast_op_le,
				*isl_ast_expr_copy*(node->u.f.iterator),
				*isl_ast_expr_copy*(node->u.f.init));
}


> I feel that I'll have to add annotations to obj_dup() as well because
> although adding an annotation to obj_cow() guarantees that the
> RetainCountChecker will not enter obj_cow's body when it is called, the
> analyzer might begin its analysis from obj_cow() which may lead it to call
> obj_dup() and result in leak warnings.
>
>
> What does obj_dup() do?
>

obj_dup() duplicates the isl object passed to it in the sense that it
copies the fiels of the passed isl object to a new object of the same type.
However, it does not change the reference count.

Generally, obj_cow() calls obj_dup(). The summary of their functionalities
is as follows.

obj_dup: copy the data, don't actually touch obj->ref
obj_cow: decrement reference count by 1 if it is > 1 and call obj_dup, or
do nothing to the reference count.

Consider the following snippet of code to understand it better.

__isl_give isl_basic_map *isl_basic_map_dup(__isl_keep isl_basic_map *bmap)
{
	struct isl_basic_map *dup;

	if (!bmap)
		return NULL;
	dup = isl_basic_map_alloc_space(isl_space_copy(bmap->dim),
			bmap->n_div, bmap->n_eq, bmap->n_ineq);
	if (!dup)
		return NULL;
	dup_constraints(dup, bmap);
	dup->flags = bmap->flags;
	dup->sample = isl_vec_copy(bmap->sample);
	return dup;

}

__attribute__((annotate("rc_ownership_trusted_implementation")))
__isl_give isl_basic_map *isl_basic_map_cow

(__isl_take isl_basic_map *bmap)
{
	if (!bmap)
		return NULL;

	if (bmap->ref > 1) {
		bmap->ref--;
		bmap = *isl_basic_map_dup*(bmap);
	}
	if (bmap) {
		ISL_F_CLR(bmap, ISL_BASIC_SET_FINAL);
		ISL_F_CLR(bmap, ISL_BASIC_MAP_REDUCED_COEFFICIENTS);
	}
	return bmap;

}


> Devin
>
>
Regards,
Malhar

ᐧ
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170716/23696f7f/attachment.html>