Re: [PATCH] Add check_result attribute

["Zack Weinberg" <zack at codesourcery dot com>]

Jakub Jelinek <jakub@redhat.com> writes:

> Hi!
>
> Some functions (e.g. in Linux kernel various uaccess.h functions/macros or
> realloc) callers should never ignore their return values, as it is either
> security problem or always a bug.
> This patch adds a function attribute so that such function can be marked
> and gcc issue warnings whenever their results are discarded.

I like the idea of being able to request this warning for functions
where it really makes sense.


> It is possible to even check result of statement expressions etc., assuming
> their return type is known, e.g. by writing:
> extern inline __attribute__((always_inline, check_result)) int
> check_int_result (int arg) { return arg; }
> #define get_user(val, addr) check_int_result (({ ... }))

Cute.  (Why not just make get_user an inline function in that case?)


> +  { "check_result",	      0, 0, true, false, false,
> +			      handle_check_result_attribute },

I would recommend a different attribute name: "warn_unused_result" or
"must_use_result".  "Check" could be anything.

> +	    if (scope && SCOPE_STMT_BLOCK (scope))
> +	      warning ("%H\
> +ignoring return value of `%D', declared with attribute check_result",
> +		       &expr_wfl_stack->location,
> +		       BLOCK_ABSTRACT_ORIGIN (SCOPE_STMT_BLOCK (scope)));
> +	    else
> +	      warning ("%H\
> +ignoring return value of function declared with attribute check_result",
> +		       &expr_wfl_stack->location);

These are awfully verbose.  Suggest just "return value of \"%D\" not used"
in both cases -- and please make both of them include the name of the
function whose result is being ignored.

> +	  && lookup_attribute ("check_result", DECL_ATTRIBUTES (fndecl)))
> +	warning ("\
> +ignoring return value of `%D', declared with attribute check_result",
> +		 fndecl);

Same here.

I don't see any obvious problems with the code, but let's hear from
other front end maintainers.

zw