Thank you for the feedback!
On Sunday, June 12th, 2022 at 7:25 AM, Prathamesh Kulkarni
<prathamesh.kulka...@linaro.org> 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 defines the
> > range of valid integer. Both min and max are inclusive and work with enums.
> > 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 <stdio.h>
> > void foo(int d) attribute ((inrange (1, 100, 200)));
> > void foo(int d) {
> > printf("%d\n", d == 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
> > [-Winrange]
> > 8 | foo(0); // warning
> > | ^~~
> >
> > I thought about having separate minval and maxval attributes but I
> > personally
> > prefer that min and max values have to be defined explicitly.
> >
> > If this looks good, I could look into applying inrange to types and
> > variables
> > 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_table[] =
> > 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 = node;
> > +
> > + / Test the position argument */
> > + tree pos = TREE_VALUE (args);
> > + if (!positional_argument (type, name, pos, INTEGER_TYPE, 0))
> > + no_add_attrs = true;
> > +
> > + / Make sure that range args are INTEGRALs */
> > + bool range_err = false;
> > + for (tree range = TREE_CHAIN (args); range; range = TREE_CHAIN (range))
> > + {
> > + tree val = TREE_VALUE (range);
> > + if (val && TREE_CODE (val) != IDENTIFIER_NODE
> > + && TREE_CODE (val) != FUNCTION_DECL)
> > + val = default_conversion (val);
> > +
> > + if (TREE_CODE (val) != 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 == 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 = true;
> > + range_err = true;
> > + }
> > + }
> > +
> > + / Test the range arg max is not smaller than min
> > + if range args are integrals */
> > + if (!range_err)
> > + {
> > + tree range = TREE_CHAIN (args);
> > + tree min = TREE_VALUE(range);
> > + range = TREE_CHAIN (range);
> > + tree max = TREE_VALUE(range);
> > + if (!tree_int_cst_le (min, max))
> > + {
> > + warning (OPT_Wattributes,
> > + "min range is bigger than max range");
> > + no_add_attrs = 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 attrs,
> > 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 = false;
> > +
> > + attrs = lookup_attribute ("inrange", attrs);
> > + if (attrs == NULL_TREE)
> > + return false;
> > +
> > + / Walk through attributes and check range values
> > + when range attribute is found /
> > +
> > + for (a = attrs; a ; a = TREE_CHAIN (a))
> > + {
> > + a = lookup_attribute ("inrange", a);
> > + tree op = TREE_VALUE (a);
> > + pos = TREE_INT_CST_LOW (TREE_VALUE (op));
> > + op = TREE_CHAIN (op);
> > + min = tree_to_shwi (TREE_VALUE (op));
> > + op = TREE_CHAIN (op);
> > + max = tree_to_shwi (TREE_VALUE (op));
> > + param = argarray[pos - 1];
> > + value = 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 = 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 has
> > 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 = check_function_nonnull (loc, TYPE_ATTRIBUTES (fntype),
> > nargs, argarray);
> >
> > + if (warn_inrange)
> > + warned_p = 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=,warn_format >= 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=,nfc,none)
> > ;
