On Tue, Jan 28, 2020 at 1:02 PM Jakub Jelinek <ja...@redhat.com> wrote: > > On Tue, Jan 28, 2020 at 05:09:36PM +0530, Prathamesh Kulkarni wrote: > > On Tue, 28 Jan 2020 at 17:00, Jakub Jelinek <ja...@redhat.com> 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<argnum>" 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 >
