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

[Torbjorn SVENSSON <torbjorn.svensson@foss.st.com>]

Ping?

On 2023-09-13 22:20, Torbjorn SVENSSON wrote:
> 
> 
> 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.