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

[Nick Alcock <nick.alcock@oracle.com>]

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*.

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).
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.)

-- 
NULL && (void)