From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 09FA6C433EF for ; Thu, 18 Nov 2021 23:15:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D883361A86 for ; Thu, 18 Nov 2021 23:15:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229833AbhKRXSw (ORCPT ); Thu, 18 Nov 2021 18:18:52 -0500 Received: from us-smtp-delivery-124.mimecast.com ([216.205.24.124]:35075 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229754AbhKRXSv (ORCPT ); Thu, 18 Nov 2021 18:18:51 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1637277349; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=lhzHVYwA6HQexy0fv3DQVriGjQYKa4W/oMur5uPdbg4=; b=PGo/1R6ZY2o91ehfCYYwcZz8Tc3+RYizzNyFjpLA5nrd2LbjCdHcWj5diooXTjrpkvxDNZ vI6Q7I2So1MQwU33RVHDDg2w2ERW6/W//hhXNw63ZRtYBzPTQbRl8OVOxrHLd9Lhj+fmSZ vRorUB4Qts0diZxes3Pn/eor0DrGODk= Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-555-BHAgVrRdPgG_BWtDjy1fcQ-1; Thu, 18 Nov 2021 18:15:48 -0500 X-MC-Unique: BHAgVrRdPgG_BWtDjy1fcQ-1 Received: by mail-qv1-f69.google.com with SMTP id q9-20020ad45749000000b003bdeb0612c5so7266918qvx.8 for ; Thu, 18 Nov 2021 15:15:48 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=lhzHVYwA6HQexy0fv3DQVriGjQYKa4W/oMur5uPdbg4=; b=LfKdSw72OMfq25e8O5uoLM/9kXBQHJcmDM+rhTfPpWpjJZN9m3xolA0m3NOlhJJoV2 Y2ZpVIVXMXcoB6p0JNus1D76U0UJAnYk8/jDrmOCRwfVLG6+u7RkZiR35eqvAFnXl9fx tzTiS4ELM1T0cynjVo0+jNY286tDqW8kyQbj4xVizXkLR6Id63gR/BVDo7/+psxKr6YV gQmmQB2Fhwko7gKo7FZaMPpNP4QyKyfSIfhvcM13zMUQBb0g6mZVSSUKNwiwolTamxn9 uuqbt9DYCzlBGmqWL9koGSsxHkuf5l3ymmx7RF82TvF+BTlovCVjXMVmNo7V2XBPeE+5 LMFQ== X-Gm-Message-State: AOAM530YpqNIHly3eOE5mIVsOWh1ptWFngQdyprmPCkf1hRABIzhIViU qN+dcZwh42aFLUcBW8majT320fFQMj4zC19AtXRZ2CPE2+XSMJiFCxJ1QZet0d9pTpf+m65rIaa GIOMygLEcFiFDtJ7BwFd1F/vI5fp/rw== X-Received: by 2002:ac8:5881:: with SMTP id t1mr1377679qta.414.1637277347966; Thu, 18 Nov 2021 15:15:47 -0800 (PST) X-Google-Smtp-Source: ABdhPJwJpccaKrVNr3rNE+euvvAEOYM9IuJnMXRL+P2FembvTiS0sMhGHQbv63rlF0U/iVtKFvM2qg== X-Received: by 2002:ac8:5881:: with SMTP id t1mr1377649qta.414.1637277347705; Thu, 18 Nov 2021 15:15:47 -0800 (PST) Received: from t14s.localdomain (c-73-69-212-193.hsd1.nh.comcast.net. [73.69.212.193]) by smtp.gmail.com with ESMTPSA id d23sm677727qkl.70.2021.11.18.15.15.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 18 Nov 2021 15:15:47 -0800 (PST) Message-ID: <08395df65e442581f2a44a786a489cf1d4c2e34c.camel@redhat.com> Subject: Re: [PATCH 2/6] Add returns_zero_on_success/failure attributes From: David Malcolm To: Prathamesh Kulkarni Cc: gcc-patches@gcc.gnu.org, linux-toolchains@vger.kernel.org Date: Thu, 18 Nov 2021 18:15:46 -0500 In-Reply-To: References: <20211113203732.2098220-1-dmalcolm@redhat.com> <20211113203732.2098220-4-dmalcolm@redhat.com> <15adb3a2a70b0d2973c30dd6d7da383ad62f413a.camel@redhat.com> User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) MIME-Version: 1.0 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmalcolm@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-toolchains@vger.kernel.org On Wed, 2021-11-17 at 14:53 +0530, Prathamesh Kulkarni wrote: > On Tue, 16 Nov 2021 at 03:42, David Malcolm > 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 > > > 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...] > > >