From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: ctfconv fix it_cmp function
To: tech@openbsd.org
Date: Thu, 22 Feb 2024 14:11:15 +0100

On Thu, Feb 22, 2024 at 01:16:16PM +0100, Martin Pieuchot wrote:
> On 22/02/24(Thu) 12:11, Claudio Jeker wrote:
> > C integer promotion bytes again. In many cases you can't use a pattern
> > like:
> > 	int diff;
> > 	if ((diff = (a->it_type - b->it_type)) != 0)
> > 		return diff;
> > 
> > The type of it_type matters and the result may not be what you think.
> > 
> > Use the pattern with explicit > and < tests instead.
> > With this the problem of duplicated types in the CTF is gone since the RB
> > tree is finally in order.
> > 
> > I'm not smart enough to know when (diff = (a - b)) is safe and when it
> > isn't so I just converted them all.
> 
> Thanks, I'm also dumb and that makes sense to me.

Actually the issue was a badly placed ) in:
	if ((a->it_type == CTF_K_INTEGER || a->it_type == CTF_K_FLOAT) &&
	    (diff = (a->it_size - b->it_size) != 0))

diff is set to 0 or 1 because the != 0 check has precedence.

Still C interger promotion is a pain and comparison functions like this
are a lot easier to read with the if (a > b) return 1; if (a < b) return -1;
style.
 
> > On top of this add a check for the encoding for CTF_K_INTEGER and
> > CTF_K_FLOAT since a signed int32_t is not equal to an unsigned int32_t.
> > On top of this I added some extra bits for the it_refp check. I doubt that
> > those matter much but better handle all the cases.
> 
> ok mpi@
> 
> > Index: parse.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/ctfconv/parse.c,v
> > diff -u -p -r1.19 parse.c
> > --- parse.c	21 Feb 2024 13:24:37 -0000	1.19
> > +++ parse.c	22 Feb 2024 10:37:49 -0000
> > @@ -323,20 +323,30 @@ it_free(struct itype *it)
> >  int
> >  it_cmp(struct itype *a, struct itype *b)
> >  {
> > -	int diff;
> > -
> > -	if ((diff = (a->it_type - b->it_type)) != 0)
> > -		return diff;
> > -
> > -	/* Basic types need to have the same size. */
> > -	if ((a->it_type == CTF_K_INTEGER || a->it_type == CTF_K_FLOAT) &&
> > -	    (diff = (a->it_size - b->it_size) != 0))
> > -		return diff;
> > +	if (a->it_type > b->it_type)
> > +		return 1;
> > +	if (a->it_type < b->it_type)
> > +		return -1;
> > +
> > +	/* Basic types need to have the same encoding and size. */
> > +	if ((a->it_type == CTF_K_INTEGER || a->it_type == CTF_K_FLOAT)) {
> > +		if (a->it_enc > b->it_enc)
> > +			return 1;
> > +		if (a->it_enc < b->it_enc)
> > +			return -1;
> > +		if (a->it_size > b->it_size)
> > +			return 1;
> > +		if (a->it_size < b->it_size)
> > +			return -1;
> > +	}
> >  
> >  	/* Arrays need to have same number of elements */
> > -	if ((a->it_type == CTF_K_ARRAY) &&
> > -	    (diff = (a->it_nelems - b->it_nelems) != 0))
> > -		return diff;
> > +	if (a->it_type == CTF_K_ARRAY) {
> > +		if (a->it_nelems > b->it_nelems)
> > +			return 1;
> > +		if (a->it_nelems < b->it_nelems)
> > +			return -1;
> > +	}
> >  
> >  	/* Match by name */
> >  	if (!(a->it_flags & ITF_ANON) && !(b->it_flags & ITF_ANON))
> > @@ -349,6 +359,10 @@ it_cmp(struct itype *a, struct itype *b)
> >  	/* Match by reference */
> >  	if ((a->it_refp != NULL) && (b->it_refp != NULL))
> >  		return it_cmp(a->it_refp, b->it_refp);
> > +	if (a->it_refp == NULL)
> > +		return -1;
> > +	if (b->it_refp == NULL)
> > +		return 1;
> >  
> >  	return 0;
> >  }
> 

-- 
:wq Claudio