From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-40140.protonmail.ch (mail-40140.protonmail.ch [185.70.40.140]) by sourceware.org (Postfix) with ESMTPS id CF55A3851ABF for ; Mon, 13 Jun 2022 12:55:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CF55A3851ABF Date: Mon, 13 Jun 2022 12:55:14 +0000 To: Prathamesh Kulkarni From: Miika Cc: "gcc@gcc.gnu.org" Reply-To: Miika Subject: [RFC] Support for nonzero attribute Message-ID: In-Reply-To: References: <6PeOfngSW_gLKMPtqp7UWs9ZQRv6KpLer221P_B9lNGGC7nJD0p-ta_7b0fwgSdz6cw7UzXsO0OGg0t2UUux81cVfIZkQcbjPwcRHFsNomI=@protonmail.com> Feedback-ID: 17471336:user:proton MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-9.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Jun 2022 12:55:22 -0000 Thank you for the feedback! On Sunday, June 12th, 2022 at 7:25 AM, Prathamesh Kulkarni wrote: > On Mon, 6 Jun 2022 at 01:39, Miika via Gcc gcc@gcc.gnu.org wrote: > > > Based on Jakub's and Yair's comments I created a new attribute "inrange= ". > > Inrage takes three arguments, pos min and max. > > Pos being the argument position in the function, and min and max define= s the > > range of valid integer. Both min and max are inclusive and work with en= ums. > > Warnings are enabled with the new flag: "-Winrange". > > > > The attribute definition would look something like this: > > inrange(pos, min, max) > > > > So for example compiling this with "gcc foo.c -Winrange": > > > > #include > > void foo(int d) attribute ((inrange (1, 100, 200))); > > void foo(int d) { > > printf("%d\n", d =3D=3D 0); > > } > > > > int main() { > > foo(0); // warning > > foo(100); // no warning > > } > > > > Would give the following error: > > > > foo.c: In function 'main': > > foo.c:8:9: warning: argument in position 1 not in rage of 100..200 [-Wi= nrange] > > 8 | foo(0); // warning > > | ^~~ > > > > I thought about having separate minval and maxval attributes but I pers= onally > > prefer that min and max values have to be defined explicitly. > > > > If this looks good, I could look into applying inrange to types and var= iables > > and after that I could start looking into optimization. > > > > Patch for adding inrange is attached below > > Hi, > Thanks for the POC patch! > A few suggestions: > > (1) It doesn't apply as-is because of transition from .c to .cc filenames= . > Perhaps you're using an older version of trunk ? I was using an older version. I should've created the patch based on the master branch. My bad! > (2) AFAIK, this warning will need an entry in doc/invoke.texi for > documenting it. Good point. I'll write up some documentation. > (3) While this patch addresses warning, I suppose it could be extended > so the tree optimizer > can take advantage of value range info provided by the attribute. > For example, the condition d > 20, could be optimized away in > > following function by inferring > range from the attribute. > > attribute((inrange (1, 10, 20))) > void foo(int d) > { > if (d > 20) > > __builtin_abort (); > } I agree. I'll try to add this too. > > Miika > > > > --- > > diff --git a/gcc/builtin-attrs.def b/gcc/builtin-attrs.def > > index 3239311b5a4..2f5732b3ed2 100644 > > --- a/gcc/builtin-attrs.def > > +++ b/gcc/builtin-attrs.def > > @@ -98,6 +98,7 @@ DEF_ATTR_IDENT (ATTR_FORMAT, "format") > > DEF_ATTR_IDENT (ATTR_FORMAT_ARG, "format_arg") > > DEF_ATTR_IDENT (ATTR_MALLOC, "malloc") > > DEF_ATTR_IDENT (ATTR_NONNULL, "nonnull") > > +DEF_ATTR_IDENT (ATTR_INRANGE, "inrange") > > DEF_ATTR_IDENT (ATTR_NORETURN, "noreturn") > > DEF_ATTR_IDENT (ATTR_NOTHROW, "nothrow") > > DEF_ATTR_IDENT (ATTR_LEAF, "leaf") > > diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c > > index ac936d5bbbb..d6dc9c37723 100644 > > --- a/gcc/c-family/c-attribs.c > > +++ b/gcc/c-family/c-attribs.c > > @@ -119,6 +119,7 @@ static tree handle_novops_attribute (tree *, tree, = tree, int, bool *); > > static tree handle_vector_size_attribute (tree *, tree, tree, int, > > bool *); > > static tree handle_nonnull_attribute (tree *, tree, tree, int, bool *); > > +static tree handle_inrange_attribute (tree *, tree, tree, int, bool *)= ; > > static tree handle_nonstring_attribute (tree *, tree, tree, int, bool *= ); > > static tree handle_nothrow_attribute (tree *, tree, tree, int, bool *); > > static tree handle_cleanup_attribute (tree *, tree, tree, int, bool *); > > @@ -379,6 +380,8 @@ const struct attribute_spec c_common_attribute_tabl= e[] =3D > > handle_tls_model_attribute, NULL }, > > { "nonnull", 0, -1, false, true, true, false, > > handle_nonnull_attribute, NULL }, > > + { "inrange", 3, 3, false, true, true, false, > > + handle_inrange_attribute, NULL }, > > { "nonstring", 0, 0, true, false, false, false, > > handle_nonstring_attribute, NULL }, > > { "nothrow", 0, 0, true, false, false, false, > > @@ -3754,6 +3757,59 @@ handle_nonnull_attribute (tree *node, tree name, > > return NULL_TREE; > > } > > > > +/* Handle the "inrange" attribute. */ > > + > > +static tree > > +handle_inrange_attribute (tree *node, tree name, > > + tree args, int ARG_UNUSED (flags), > > + bool *no_add_attrs) > > +{ > > + tree type =3D node; > > + > > + / Test the position argument */ > > + tree pos =3D TREE_VALUE (args); > > + if (!positional_argument (type, name, pos, INTEGER_TYPE, 0)) > > + no_add_attrs =3D true; > > + > > + / Make sure that range args are INTEGRALs */ > > + bool range_err =3D false; > > + for (tree range =3D TREE_CHAIN (args); range; range =3D TREE_CHAIN (r= ange)) > > + { > > + tree val =3D TREE_VALUE (range); > > + if (val && TREE_CODE (val) !=3D IDENTIFIER_NODE > > + && TREE_CODE (val) !=3D FUNCTION_DECL) > > + val =3D default_conversion (val); > > + > > + if (TREE_CODE (val) !=3D INTEGER_CST > > + || !INTEGRAL_TYPE_P (TREE_TYPE (val))) > > Um, why the check for INTEGRAL_TYPE_P here ? > IIUC, this will also accept non-constant integer values. > For eg, the following compiles without any warning: > int a; > int b; > > void foo(int d) attribute ((inrange (1, a, b))); > void foo(int d) { > __builtin_printf("%d\n", d =3D=3D 0); > } > > Is this intended ? This was intended behavior but now that I think about it, it's probably best to just use constant integers. Good catch! > > + { > > + warning (OPT_Wattributes, > > + "range value is not an integral constant"); > > + no_add_attrs =3D true; > > + range_err =3D true; > > + } > > + } > > + > > + / Test the range arg max is not smaller than min > > + if range args are integrals */ > > + if (!range_err) > > + { > > + tree range =3D TREE_CHAIN (args); > > + tree min =3D TREE_VALUE(range); > > + range =3D TREE_CHAIN (range); > > + tree max =3D TREE_VALUE(range); > > + if (!tree_int_cst_le (min, max)) > > + { > > + warning (OPT_Wattributes, > > + "min range is bigger than max range"); > > + no_add_attrs =3D true; > > + return NULL_TREE; > > + } > > + } > > + > > + return NULL_TREE; > > +} > > + > > / Handle the "nonstring" variable attribute. */ > > > > static tree > > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c > > index 20258c331af..8936942fec8 100644 > > --- a/gcc/c-family/c-common.c > > +++ b/gcc/c-family/c-common.c > > @@ -5342,6 +5342,51 @@ check_function_nonnull (location_t loc, tree att= rs, int nargs, tree *argarray) > > return ctx.warned_p; > > } > > > > + > > +/* Check the argument list of a function call for invalid range > > + in argument slots that are marked as requiring a specific range. > > + Return true if we have warned. */ > > + > > +static bool > > +check_function_inrange (location_t loc, tree attrs, tree argarray) > > +{ > > + tree a; > > + tree param; > > + int pos; > > + HOST_WIDE_INT min; > > + HOST_WIDE_INT max; > > + HOST_WIDE_INT value; > > + bool warned =3D false; > > + > > + attrs =3D lookup_attribute ("inrange", attrs); > > + if (attrs =3D=3D NULL_TREE) > > + return false; > > + > > + / Walk through attributes and check range values > > + when range attribute is found / > > + > > + for (a =3D attrs; a ; a =3D TREE_CHAIN (a)) > > + { > > + a =3D lookup_attribute ("inrange", a); > > + tree op =3D TREE_VALUE (a); > > + pos =3D TREE_INT_CST_LOW (TREE_VALUE (op)); > > + op =3D TREE_CHAIN (op); > > + min =3D tree_to_shwi (TREE_VALUE (op)); > > + op =3D TREE_CHAIN (op); > > + max =3D tree_to_shwi (TREE_VALUE (op)); > > + param =3D argarray[pos - 1]; > > + value =3D TREE_INT_CST_LOW (param); > > + if (value < min || value > max) > > + { > > + warning_at (loc, OPT_Winrange, "argument in position %u" > > + " not in rage of %ld..%ld", pos, min, max); > > + warned =3D true; > > + } > > + } > > + > > + return warned; > > +} > > + > > / Check that the Nth argument of a function call (counting backwards > > from the end) is a (pointer)0. The NARGS arguments are passed in the > > array ARGARRAY. */ > > @@ -5703,7 +5748,7 @@ attribute_fallthrough_p (tree attr) > > > > /* Check for valid arguments being passed to a function with FNTYPE. > > There are NARGS arguments in the array ARGARRAY. LOC should be used > > - for diagnostics. Return true if either -Wnonnull or -Wrestrict has > > + for diagnostics. Return true if -Winrange, -Wnonnull or -Wrestrict ha= s > > been issued. > > > > The arguments in ARGARRAY may not have been folded yet (e.g. for C++, > > @@ -5723,6 +5768,10 @@ check_function_arguments (location_t loc, const_= tree fndecl, const_tree fntype, > > warned_p =3D check_function_nonnull (loc, TYPE_ATTRIBUTES (fntype), > > nargs, argarray); > > > > + if (warn_inrange) > > + warned_p =3D check_function_inrange (loc, TYPE_ATTRIBUTES (fntype), > > + argarray); > > + > > /* Check for errors in format strings. */ > > > > if (warn_format || warn_suggest_attribute_format) > > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > > index c49da99d395..0b9aa597c54 100644 > > --- a/gcc/c-family/c.opt > > +++ b/gcc/c-family/c.opt > > @@ -932,6 +932,14 @@ Wnonnull-compare > > C ObjC C++ ObjC++ LangEnabledBy(C ObjC C++ ObjC++,Wall) > > ; > > > > +Winrange > > +C Var(warn_inrange) Warning LangEnabledBy(C,Wformat=3D,warn_format >= =3D 1,0) > > +Warn about integer not being in specified range. > > + > > +Winrange > > +C LangEnabledBy(C,Wall) > > Just curious, why only C ? I wasn't sure how much extra work it would've been to support ObjC C++ and = ObjC++ so I introduced the concept only with C support. Now that I know that this is a wanted feature, I'll add support for the other languages too. > Thanks, > Prathamesh > > > +; > > + > > Wnormalized > > C ObjC C++ ObjC++ Warning Alias(Wnormalized=3D,nfc,none) > > ;