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

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

On 2023-09-28 18:41, Nick Alcock wrote:
> On 26 Sep 2023, Torbjorn SVENSSON said:
> 
>> On 2023-09-26 16:51, Nick Alcock wrote:
>>> On 13 Sep 2023, Torbjorn SVENSSON outgrape:
>>>> On 2023-09-13 20:37, Nick Alcock wrote:
>>>>> On 13 Sep 2023, Torbjörn SVENSSON verbalised:
>>> Honestly I suspect all we need is a better name:
>>> ctf_set_int_errno(...);
>>> ctf_set_type_errno(...)
>>> and then use one or the other, consistently. (Neither needs to call the
>>> other: they're only two lines long!)
>>
>> Ok. I've updated the patch (V4) to be like you suggested above.
> 
> Thanks!
> 
>>>> I suppose the ctf_set_errno_unsigned could even be a macro in the ctf-impl.h header file.
>>> I'd make both of them inline functions personally (I bet it would reduce
>>> code size!)
>>
>> I do not see any major difference in code size for the ld.exe binary after the change.
> 
> Oh well, it was just a pious hope really.
> 
>>>>>> +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.
>>> ... sizeof(signed int) > sizeof(unsigned long int)?! Is that even
>>> possible? I would have assumed from the C type hierarchy and the integer
>>> conversion rank rules would have required that unsigned long int was at
>>> least as big as any non-long integral type, but I don't see anywhere
>>> it's required in the standard, dammit...
>>
>> I don't know about the 'sizeof(signed int) > sizeof(unsigned long
>> int)' part, but what I said was _integral type_, not _int_. In the
> 
> Ah true. My apologies.
> 
>> case where I saw the problem, it was ssize_t but I'm not sure what
>> that maps to, but it's wider than unsigned long int apparently in this
>> case.
> 
> Aha! So this is *not* a problem with functions returning int -- it is
> specifically a problem with functions returning *size_t types*.
> 

So, I'm not sure if were are talking past each other, but I don't think 
it's a *size_t type* issue either.

As I see it, there are 3 different scenarios.

1. The function returns a signed integral type, then -1 would be fine. 
Even a simple (int)-1 would work as it would be sign-extended.

2. The function returns an unsigned integral type that is as wide, or 
wider, than unsigned long. In this case, CTF_ERR will be zero extended 
and the value will be UINT_something_MAX (the number of binary ones 
depends on the sizeof(unsigned long)).

3. The function return an unsigned integral type that is narrower than 
unsigned long. In this case, the cast to unsigned long is wider than the 
return type and may overflow(?). Is it safe to return something bigger 
than can be represented in the return type?

I don't know if the third option above is applicable as I have not 
checked the width of the types that is calling the ctf_set_errno() 
function before my patch, but the other 2 are there.

In my mind, I see it as we need 2 different implementations. One that 
returns a simple -1 and another one that casts it for all unsigned 
calls, but maybe I'm oversimplifying this.


To make things easier, I would actually consider just having the assign 
statement and the return statement inlined (without a macro or inline 
function) directly where they are supposed to be.
Would you be open to removing the ctf_set_errno() function completely 
and just expand it to where it's called (almost like my v2 patch, but 
everywhere instead)?
Alternatively, we could start the discussion on adopting the C11 
standard instead, but I fear that this will be a much longer discussion 
than figuring out what the best approach would be for libctf.


> My apologies, I misunderstood the entire problem.  We probably *do*
> still want ctf_set_errno_signed for functions returning int (for clarity
> if nothing else), but for ssize_t in particular this won't do: we
> probably want a ctf_set_errno_ssize_t or something. The name is awful
> but I wasted a day failing to think of a better one :(
> 
> There are very few functions returning (s)size_t in libctf:
> 
> extern size_t ctf_archive_count (const ctf_archive_t *);
> extern ssize_t ctf_type_lname (ctf_dict_t *, ctf_id_t, char *, size_t);
> extern ssize_t ctf_type_size (ctf_dict_t *, ctf_id_t);
> extern ssize_t ctf_type_align (ctf_dict_t *, ctf_id_t);
> extern ssize_t ctf_member_next (ctf_dict_t *, ctf_id_t, ctf_next_t **,
> 				const char **name, ctf_id_t *membtype,
> 				int flags);
> 
> Of these, ctf_archive_count () cannot fail, so the problem reduces to
> ssize_t alone.  These functions should probably
> 
>      return (ssize_t) ctf_set_errno_signed (...))
> 
> (it's rare enough that a utility functions to do this is probably
> unnecessary).
> 
> We also have (in ctf_type_lname):
> 
>    if (str == NULL)
>      return CTF_ERR;			/* errno is set for us.  */
> 
> This should probably become a straight -1 (no cast necessary).

Yes, this is another place where the problem appears.
Looks like the -Wsign-conversion does not warn when it's the return 
statement for the function. Doing a simple grep in the tree reveals 56 
places that needs to be verified.

> ctf_type_size () already gets this right (but needs _ssize_t adjustments
> to its ctf_set_errno () calls, as does get_vbytes_common in ctf-open.c).
> The same is true of ctf_type_align (), and, of course, ctf_member_next
> ().
> 
>>>>> 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.
>>> OK, I'm back from various conferences so I can throw tests past this at
>>> any time, it's largely automated. So once I stop faffing about and
>>> changing my mind and we converge on something I'll throw it past every
>>> test I've got. (It takes a day or so.)
>>
>> If you do not see any problem with the V4 patch, then please go ahead
>> and run the tests that you have to get a verdict.
> 
> ... sorry, I'm still flailing at it. Maybe the above is helpful? (It's
> only a very small change atop what you've already done, I think.)
> 

Let me know how you want to handle this. :)

Kind regards,
Torbjörn