On 6/13/22 06:55, Miika via Gcc wrote:
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 belowHi, 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.
The attribute handling can be (and to benefit optimization needs to be) fully implemented in the middle end. There is no need to change any of the front ends. Martin
Thanks, Prathamesh+; + Wnormalized C ObjC C++ ObjC++ Warning Alias(Wnormalized=,nfc,none) ;
