On Wed, 2021-11-17 at 14:53 +0530, Prathamesh Kulkarni wrote: > On Tue, 16 Nov 2021 at 03:42, David Malcolm <dmalc...@redhat.com> > wrote: > > > > On Mon, 2021-11-15 at 12:33 +0530, Prathamesh Kulkarni wrote: > > > On Sun, 14 Nov 2021 at 02:07, David Malcolm via Gcc-patches > > > <gcc-patches@gcc.gnu.org> wrote: > > > > > > > > This patch adds two new attributes. The followup patch makes > > > > use of > > > > the attributes in -fanalyzer. > > > > [...snip...] > > > > > > +/* 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. > > > > > 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. > > > > > > > > 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 ?
Perhaps. All of this sounds like scope creep beyond my immediate requirements though :) > > > > 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 ? Likewise - sounds potentially useful, but I don't need this for this kernel trust boundaries patch kit. Dave > > Thanks, > Prathamesh > > > > > 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 > > > > > > > > > > Thanks, > > > Prathamesh > > > > [...snip...] > > >