From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 33968 invoked by alias); 12 Feb 2020 08:35:11 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 33928 invoked by uid 89); 12 Feb 2020 08:35:08 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy=H*f:sk:fYTmM2M, H*i:sk:fYTmM2M X-HELO: mail-lf1-f65.google.com Received: from mail-lf1-f65.google.com (HELO mail-lf1-f65.google.com) (209.85.167.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 12 Feb 2020 08:35:06 +0000 Received: by mail-lf1-f65.google.com with SMTP id b15so976359lfc.4 for ; Wed, 12 Feb 2020 00:35:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=OuAUHeRzXEbfH0+zcdFMROFI20NZ5SczxzxiHbtQdI0=; b=orEKhQYAKhXtqbUg+XzwIJ7eqOCPXnn4J2IvVXMh5xYYYHIBnbYvBiYCEFOXbWRDIp dfRRgjwpcJN6CLlAsJo2e/6Sz0vgj6i14NwX9e2F7ZtFlNhsdSN9yVpfRUTW7Ft2YpVk PSdqFoRtEZXs5noJbgh7zPnP3JHj6owJbobZukyHPq2YKi0yi38WJHm+0jiV8r471X0Z tZ6RTJ+9z8wTodiK2DDLgfPc356Ld1MQHnfGVx/Cf+QtswgleqZQnxRQtvusvVR2TOD2 reYs8pYRwmQ2nU3DeZFyXLI9VnwT4+cbv7UqGrjTTad2uiF+xLNYOSUUNt06XVgOOrb2 +xBg== MIME-Version: 1.0 References: <20200128113036.GX17695@tucnak> <20200128120152.GY17695@tucnak> In-Reply-To: From: Richard Biener Date: Wed, 12 Feb 2020 08:35:00 -0000 Message-ID: Subject: Re: [RFC] [c-family] PR92867 - Add returns_arg attribute To: Prathamesh Kulkarni Cc: Jakub Jelinek , Joseph Myers , gcc Patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2020-02/txt/msg00715.txt.bz2 On Wed, Feb 12, 2020 at 7:52 AM Prathamesh Kulkarni wrote: > > On Tue, 4 Feb 2020 at 14:54, Prathamesh Kulkarni > wrote: > > > > On Mon, 3 Feb 2020 at 14:56, Prathamesh Kulkarni > > wrote: > > > > > > On Mon, 3 Feb 2020 at 14:41, Prathamesh Kulkarni > > > wrote: > > > > > > > > On Thu, 30 Jan 2020 at 19:17, Richard Biener wrote: > > > > > > > > > > On Thu, Jan 30, 2020 at 11:49 AM Prathamesh Kulkarni > > > > > wrote: > > > > > > > > > > > > On Wed, 29 Jan 2020 at 14:38, Richard Biener wrote: > > > > > > > > > > > > > > On Tue, Jan 28, 2020 at 1:02 PM Jakub Jelinek wrote: > > > > > > > > > > > > > > > > On Tue, Jan 28, 2020 at 05:09:36PM +0530, Prathamesh Kulkarni wrote: > > > > > > > > > On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek wrote: > > > > > > > > > > > > > > > > > > > > On Tue, Jan 28, 2020 at 04:56:59PM +0530, Prathamesh Kulkarni wrote: > > > > > > > > > > > Thanks for the suggestions. In the attached patch I bumped up value of > > > > > > > > > > > ERF_RETURNS_ARG_MASK > > > > > > > > > > > to UINT_MAX >> 2, and use highest two bits for ERF_NOALIAS and ERF_RETURNS_ARG. > > > > > > > > > > > And use fn spec "Z" to store the argument number in fn-spec format. > > > > > > > > > > > Does that look OK ? > > > > > > > > > > > > > > > > > > > > No. > > > > > > > > > > > > > > > > > > > > +#define ERF_RETURN_ARG_MASK (UINT_MAX >> 2) > > > > > > > > > > > > > > > > > > > > /* Nonzero if the return value is equal to the argument number > > > > > > > > > > flags & ERF_RETURN_ARG_MASK. */ > > > > > > > > > > -#define ERF_RETURNS_ARG (1 << 2) > > > > > > > > > > +#define ERF_RETURNS_ARG (1 << (BITS_PER_WORD - 2)) > > > > > > > > > > > > > > > > > > > > How is size of host int related to BITS_PER_WORD? Not to mention that > > > > > > > > > > if BITS_PER_WORD is 64 and host int is 32-bit, 1 << (64 - 2) is UB. > > > > > > > > > Oops sorry. I should have used HOST_BITS_PER_INT. > > > > > > > > > I assume that'd be correct ? > > > > > > > > > > > > > > > > It still wouldn't, 1 << (HOST_BITS_PER_INT - 1) is negative number, you'd > > > > > > > > need either 1U and verify all ERF_* flags uses, or avoid using the sign bit. > > > > > > > > The patch has other issues, you don't verify that the argnum fits into > > > > > > > > the bits (doesn't overflow into the other ERF_* bits), in > > > > > > > > + char *s = (char *) xmalloc (sizeof (char) * BITS_PER_WORD); > > > > > > > > + s[0] = 'Z'; > > > > > > > > + sprintf (s + 1, "%lu", argnum); > > > > > > > > 1) sizeof (char) is 1 by definition > > > > > > > > 2) it is pointless to allocate it and then deallocate (just use automatic > > > > > > > > array) > > > > > > > > 3) it is unclear how is BITS_PER_WORD related to the length of decimal > > > > > > > > encoded string + Z char + terminating '\0'. The usual way is for unsigned > > > > > > > > numbers to estimate number of digits by counting 3 digits per each 8 bits, > > > > > > > > in your case of course + 2. > > > > > > > > > > > > > > > > More importantly, the "fn spec" attribute isn't used just in > > > > > > > > gimple_call_return_flags, but also in e.g. gimple_call_arg_flags which > > > > > > > > assumes that the return stuff in there is a single char and the reaming > > > > > > > > chars are for argument descriptions, or in decl_return_flags which you > > > > > > > > haven't modified. > > > > > > > > > > > > > > > > I must say I fail to see the point in trying to glue this together into the > > > > > > > > "fn spec" argument so incompatibly, why can't we handle the fn spec with its > > > > > > > > 1-4 returns_arg and returns_arg attribute with arbitrary position > > > > > > > > side-by-side? > > > > > > > > > > > > > > Yeah, I wouldn't have added "fn spec" for "returns_arg" but kept the > > > > > > > query interface thus access it via gimple_call_return_flags and use > > > > > > > ERF_*. For the flags adjustment just up the maximum argument > > > > > > > to 1<<15 then the argument number is also nicely aligned, no need > > > > > > > to do fancy limiting that depends on the host. For too large > > > > > > > argument numbers just warn the attribute is ignored. I'd say even > > > > > > > a max of 255 is sane just the existing limit is a bit too low. > > > > > > Hi, > > > > > > Thanks for the suggestions. In the attached patch, I use TREE_VALUE > > > > > > (attr) to store/retrieve > > > > > > arbitrary argument position, and have bumped ERF_RETURNS_ARG_MASK to 0x3fff. > > > > > > Does it look OK ? > > > > > > > > > > + warning_at (DECL_SOURCE_LOCATION (decl), OPT_Wattributes, > > > > > + "%qE attribute ignored on a function returning %qT.", > > > > > + name, rettype); > > > > > > > > > > no punctuation in diagnostics (trailing '.'), also elsewhere in the patch. > > > > > > > > > > + tree fndecl = gimple_call_fndecl (stmt); > > > > > + attr = lookup_attribute ("returns_arg", DECL_ATTRIBUTES (fndecl)); > > > > > + if (attr) > > > > > + { > > > > > + unsigned argnum = tree_to_uhwi (TREE_VALUE (TREE_VALUE (attr))); > > > > > + return ERF_RETURNS_ARG | argnum; > > > > > > > > > > please verify argnum against ERF_ARG_MASK. > > > > > > > > > > + tree attr = lookup_attribute ("returns_arg", DECL_ATTRIBUTES (decl)); > > > > > + if (attr) > > > > > + TREE_VALUE (attr) = build_int_cst (unsigned_type_node, argnum); > > > > > + else > > > > > + DECL_ATTRIBUTES (decl) > > > > > + = tree_cons (get_identifier ("returns_arg"), > > > > > + build_int_cst (unsigned_type_node, argnum), > > > > > + DECL_ATTRIBUTES (decl)); > > > > > + return NULL_TREE; > > > > > > > > > > what's this for? for *no_add_attrs = false you get the attribute > > > > > added by the caller. > > > > > Also other positional_argument callers overwrite TREE_VALUE with the result > > > > > from the function. > > > > Ah, thanks for pointing out! > > > > Does attached version look OK ? > > > Oops sorry, the previous patch contained typo ERF_RETURNS_ARG_MASK > > > (should be ERF_RETURN_ARG_MASK). > > > Fixed in this patch. > > Validating previous patch reported a segfault because > > gimple_call_fndecl returned NULL in gimple_call_return_flags. > > Fixed in attached patch. > > Does it look OK ? > ping https://gcc.gnu.org/ml/gcc-patches/2020-02/msg00159.html Queued for review, it's stage1 material anyways. Richard. > Thanks, > Prathamesh > > > > Thanks, > > Prathamesh > > > > > > Thanks, > > > Prathamesh > > > > > > > > Thanks, > > > > Prathamesh > > > > > > > > > > > Thanks, > > > > > > Prathamesh > > > > > > > > > > > > > > Richard. > > > > > > > > > > > > > > > Jakub > > > > > > > >