From: Martin Pieuchot <mpi@openbsd.org>
Subject: Re: ctfconv fix it_cmp function
To: tech@openbsd.org
Date: Thu, 22 Feb 2024 13:16:16 +0100

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.

> 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;
>  }