Re: [PATCH 2/6] Add returns_zero_on_success/failure attributes

On Tue, 16 Nov 2021 at 03:42, David Malcolm [off-list ref] wrote:

On Mon, 2021-11-15 at 12:33 +0530, Prathamesh Kulkarni wrote:

quoted

On Sun, 14 Nov 2021 at 02:07, David Malcolm via Gcc-patches
[off-list ref] wrote:

quoted

This patch adds two new attributes.  The followup patch makes use of
the attributes in -fanalyzer.

[...snip...]

quoted

quoted

+/* Handle "returns_zero_on_failure" and "returns_zero_on_success"
attributes;
+   arguments as in struct attribute_spec.handler.  */
+
+static tree
+handle_returns_zero_on_attributes (tree *node, tree name, tree,
int,
+                                  bool *no_add_attrs)
+{
+  if (!INTEGRAL_TYPE_P (TREE_TYPE (*node)))
+    {
+      error ("%qE attribute on a function not returning an
integral type",
+            name);
+      *no_add_attrs = true;
+    }
+  return NULL_TREE;

Hi David,

Thanks for the ideas.

quoted

Just curious if a warning should be emitted if the function is marked
with the attribute but it's return value isn't actually 0 ?

That sounds like a worthwhile extension of the idea.  It should be
possible to identify functions that can't return zero or non-zero that
have been marked as being able to.

That said:

(a) if you apply the attribute to a function pointer for a callback,
you could have an implementation of the callback that always fails and
returns, say, -1; should the warning complain that the function has the
"returns_zero_on_success" property and is always returning -1?

Ah OK. In that case, emitting a diagnostic if the return value
isn't 0, doesn't make sense for "returns_zero_on_success" since the
function "always fails".
Thanks for pointing out!

(b) the attributes introduce a concept of "success" vs "failure", which
might be hard for a machine to determine.  It's only used later on in
terms of the events presented to the user, so that -fanalyzer can emit
e.g. "when 'copy_from_user' fails", which IMHO is easier to read than
"when 'copy_from_user' returns non-zero".

Indeed.

quoted

There are other constants like -1 or 1 that are often used to indicate
error, so maybe tweak the attribute to
take the integer as an argument ?
Sth like returns_int_on_success(cst) / returns_int_on_failure(cst) ?

Those could work nicely; I like the idea of them being supplementary to
the returns_zero_on_* ones.

I got the urge to bikeshed about wording; some ideas:
  success_return_value(CST)
  failure_return_value(CST)
or maybe additionally:
  success_return_range(LOWER_BOUND_CST, UPPER_BOUND_CST)
  failure_return_range(LOWER_BOUND_CST, UPPER_BOUND_CST)

Extending to range is a nice idea ;-)
Apart from success / failure, if we just had an attribute
return_range(low_cst, high_cst), I suppose that could
be useful for return value optimization ?

I can also imagine a
  sets_errno_on_failure
attribute being useful (and perhaps a "doesnt_touch_errno"???)

More generally, would it be a good idea to provide attributes for
mod/ref anaylsis ?
So sth like:
void foo(void) __attribute__((modifies(errno)));
which would state that foo modifies errno, but neither reads nor
modifies any other global var.
and
void bar(void) __attribute__((reads(errno)))
which would state that bar only reads errno, and doesn't modify or
read any other global var.
I guess that can benefit optimization, since we can have better
context about side-effects of a function call.
For success / failure context, we could add attributes
modifies_on_success, modifies_on_failure ?

Thanks,
Prathamesh

quoted

Also, would it make sense to extend it for pointers too for returning
NULL on success / failure ?

Possibly expressible by generalizing it to allow pointer types, or by
adding this pair:

  returns_null_on_failure
  returns_null_on_success

or by using the "range" idea above.

In terms of scope, for the trust boundary stuff, I want to be able to
express the idea that a call can succeed vs fail, what the success vs
failure is in terms of nonzero vs zero, and to be able to wire up the
heuristic that if it looks like a "copy function" (use of access
attributes and a size), that success/failure can mean "copies all of
it" vs "copies none of it" (which seems to get decent test coverage on
the Linux kernel with the copy_from/to_user fns).

Thanks
Dave

quoted

Thanks,
Prathamesh

[...snip...]