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

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

On 13 Sep 2023, Torbjorn SVENSSON outgrape:

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

Oh right, that should work (given a platform on which this goes wrong in
the first place).

I should add -Wconversion to my test flags...

>>> +/* 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?

Simply because I foolishly assumed that CTF_ERR would always end up
== -1 even when passed through a function returning int. This is, uh,
not true.

I do wish that C was defined such that we had one consistent type we
could compare with for errors and not have to worry, but we don't:
functions returning a ctf_id_t return CTF_ERR on error, functions
returning an int return -1 on error and that's just the way it is :(
(This was always true, even in the Solaris era, but when ctf_id_t was an
int this was less visible than it is now.)

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

Yeah.

> 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

... and then all the ctf_id_t-returning functions call that?

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

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

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

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

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

-- 
NULL && (void)