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?
Jakub
