From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-il1-x12c.google.com (mail-il1-x12c.google.com [IPv6:2607:f8b0:4864:20::12c]) by sourceware.org (Postfix) with ESMTPS id 73B7B382B25A for ; Mon, 13 Jun 2022 16:02:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 73B7B382B25A Received: by mail-il1-x12c.google.com with SMTP id y16so4543609ili.13 for ; Mon, 13 Jun 2022 09:02:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=IkSf0bppIPFTHvMZ03aldB7U9itFQ3k5zLGVk4XzEKQ=; b=E7XMPB5ZuM36CNHwsSvelS6fTuxsmdNwXygYfIAQTXl9geeggICdpBhbN9jxouVy9G a2rR+tQU+F7jULlG6zMCXms2amSD/pRk3WqvB+HxLhmOl18EXJdb7EFx4u1E1x0ZMKny DwAKKhPojj6uKZ1qkmBFXU/DJU0eXx3GNbrciTNjflSxR0Y1cZtaZQgLronsd61WqxVN 6nesDmMAY5RT8TLCCWwwJESJw9mItPaX/zQOKDle3FEay8iWwO71PsEGiLUg6b1xSDCz as8QBB0qz9evChed0AaUGhoAhA8/pPAhkdToQshy4ZdZSeLZhWsZueoIOxJpZY5NipUG K6Fg== X-Gm-Message-State: AJIora9VXB0pUklNOBT5kcgvGz4s7Jr0l9QJmakxLrUwwJP1gB1UoL9u yx3A1814UQXyxKy6OL9iIZRAcNUGaVQrgg== X-Google-Smtp-Source: AGRyM1tUfT0mG453FBznxk47Um6XwLloMSL1C6u+dUD4Gq/C9ZopF6YHuEdcckTWlFBI2Crxu1yo/Q== X-Received: by 2002:a05:6e02:1c0d:b0:2d3:c2f2:3cb4 with SMTP id l13-20020a056e021c0d00b002d3c2f23cb4mr317034ilh.68.1655136152466; Mon, 13 Jun 2022 09:02:32 -0700 (PDT) Received: from [192.168.0.41] (97-118-121-109.hlrn.qwest.net. [97.118.121.109]) by smtp.gmail.com with ESMTPSA id v2-20020a056602014200b0066a0c0beee7sm118933iot.44.2022.06.13.09.02.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 13 Jun 2022 09:02:32 -0700 (PDT) Message-ID: <0f63f01e-9678-020f-ab91-2195ba2302bd@gmail.com> Date: Mon, 13 Jun 2022 10:02:30 -0600 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 Subject: Re: [RFC] Support for nonzero attribute Content-Language: en-US To: Miika , Prathamesh Kulkarni Cc: "gcc@gcc.gnu.org" References: <6PeOfngSW_gLKMPtqp7UWs9ZQRv6KpLer221P_B9lNGGC7nJD0p-ta_7b0fwgSdz6cw7UzXsO0OGg0t2UUux81cVfIZkQcbjPwcRHFsNomI=@protonmail.com> From: Martin Sebor In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, 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 16:02:44 -0000 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 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 >>> 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. 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) >>> ;