From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 59702 invoked by alias); 29 Jan 2020 09:08:42 -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 59691 invoked by uid 89); 29 Jan 2020 09:08:42 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mail-lf1-f68.google.com Received: from mail-lf1-f68.google.com (HELO mail-lf1-f68.google.com) (209.85.167.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 29 Jan 2020 09:08:40 +0000 Received: by mail-lf1-f68.google.com with SMTP id t23so11327709lfk.6 for ; Wed, 29 Jan 2020 01:08:40 -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=SHbR1dVFWpKUYBhw/eFYPaZBfaSgzJdk8TKU4s+hxBc=; b=SFIy5fifmYgYTjG2c17U7MUNQNsZp7QsjH93DgZufRZd4XeA7MjMzXRlN9s3z3y1ZE Lwde641pXZ11gHxVqgLeO3ur4I5XYRVtJ2y3wwRwwfiILlOJSrUTsCgvCvLQHVXkLU9v nrnNQWYvKrdqj2BjcK65/rd5eqvEUd63eAqd8iuh26hcaYOKAB57SyjJiY6g5KaCYYDA Xv2hG3NfBPtoCeMTiebPT6wBOKIroRuE+d7DJihD14QXhI90dJayn1KLSktDZJOAF3QY Y+N/c9ZF36ZxHxAXYSYuUs8snSoKc3aw32Lf32xMBPZckN3tvkeA3+Q4/lNgHDhz0pVH AA5A== MIME-Version: 1.0 References: <20200128113036.GX17695@tucnak> <20200128120152.GY17695@tucnak> In-Reply-To: <20200128120152.GY17695@tucnak> From: Richard Biener Date: Wed, 29 Jan 2020 10:06:00 -0000 Message-ID: Subject: Re: [RFC] [c-family] PR92867 - Add returns_arg attribute To: Jakub Jelinek Cc: Prathamesh Kulkarni , Joseph Myers , gcc Patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2020-01/txt/msg01914.txt.bz2 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. Richard. > Jakub >