[PATCH v3] libctf: ctf_member_next needs to return (ssize_t)-1 on error

[Torbjorn SVENSSON]

On 2023-09-13 20:37, Nick Alcock wrote:
> On 13 Sep 2023, Torbjörn SVENSSON verbalised:
> 
>> v1 -> v2:
>> Changed all functions with signed interger return type to return -1 based on
>> comment from Alan.
>>
>> v2 -> v3:
>> Added ctf_set_errno_signed function to return a signed -1 value based on
>> comment from Nick.
>>
>> Ok for trunk?
> 
> If this touches exactly those functions that return int, and fixes the
> reported bug, it's good as far as I'm concerned, except for a couple of
> possible comment improvements:

I've verified the calls by building binutils (with the configure flags 
mentioned in my last mail) with CFLAGS="-Wsign-conversion -Wconversion" 
and looking for any warnings related to ctf_set_errno. After applying 
this patch, there were no warnings left.

>> +/* Store the specified error code into the CTF dict, and then return -1
>> +   (CTF_ERR) for the benefit of the caller. */
> 
> It's not CTF_ERR in this case, it's just -1. Perhaps:

True, but why is then ctf_set_errno returning CTF_ERR?
I somehow want to make it obvious that it's not wrong and that it should 
*never* be CTF_ERR in the signed function or the problem would reappear.

The other possibility is to do the inverse, meaning that the 
ctf_set_errno function is returning an integer (-1) and that there is a 
function ctf_set_errno_unsigned that is calling the ctf_set_errno 
function but casting the returned value to unsigned long (or ctf_id_t). 
I personally think this solution is a bit more clean as -1 is the error 
value from all functions, just a matter if it's signed or unsigned.

I.e:

int
ctf_set_errno (ctf_dict_t *fp, int err)
{
   fp->ctf_errno = err;
   return -1;
}

unsigned long
ctf_set_errno_unsigned (ctf_dict_t *fp, int err)
{
   return (unsigned long)ctf_set_errno (fp, err);
}

I suppose the ctf_set_errno_unsigned could even be a macro in the 
ctf-impl.h header file.



> /* Store the specified error code into the CTF dict, and then return -1
>     for the benefit of the caller, which is expected to return int,
>     as opposed to ctf_id_t. */
> 

Ok!

>> +int
>> +ctf_set_errno_signed (ctf_dict_t *fp, int err)
>> +{
>> +  fp->ctf_errno = err;
>> +  /* Don't rely on CTF_ERR here as it will not properly sign extend on 64-bit
>> +     Windows ABI.  */
>> +  return -1;
>> +}
> 
> ... that Windows is not really the problem here. It's more
> 
> /* Don't rely on CTF_ERR here; it is a ctf_id_t (unsigned long), and
>     it will be truncated to a non--1 value on platforms on which int
>     and unsigned long are different sizes.  */
> 
> perhaps? (At least, I think that's what's going on.)

The problem happens when the signed integral type is wider than unsigned 
long.

  /* Don't rely on CTF_ERR here; it is a ctf_id_t (unsigned long), and
      it will be extended to a non--1 value on platforms on which int
      is larger than unsigned long are different sizes.  */

> 
> This probably needs testing on a wide variety of platforms with
> different type sizes. I'll add throwing this through my entire test
> matrix to my todo list, and fix any bugs observed: but the basic idea
> looks sound to me.

Do you want to run this full matrix before or after submitting the patch?
If it's before; when do you think you will have time to do that?


Let me know how you want to proceed.