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

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

Hello Nick,


On 2023-10-02 12:57, Nick Alcock wrote:
> On 29 Sep 2023, Torbjorn SVENSSON verbalised:
> 
>> On 2023-09-28 18:41, Nick Alcock wrote:
>>> 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.
> 
> Ack.
> 
>> 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)).
> 
> Ack. This is the problem area, because the interface contract for libctf
> specifies (or would if it were written down anywhere) that you can test
> all signed returns for error via < 0 and all unsigned ones (like
> ctf_id_t) via == CTF_ERR, and CTF_ERR... has a cast in it.

I don't think this is true with the current logic in place.
On a signed integral function, it could be either -1 (negative one) or, 
if the data type of the called function is wide enough, CTF_ERR.

> I suppose if we changed CTF_ERR to (uintmax_t)-1 this might work for all
> cases, but a) uintmax_t is an annoying sod that doesn't even extend to
> the largest possible unsigned integral type on all platforms and b) this
> just gives us the same problem from the other side if the unsigned type
> is shorter than uintmax_t (which it almost always would be). What would
> happen to the (unsigned long)-1 the libctf function returned when it was
> promoted? Would it turn into (uintmax_t)-1 consistently, given that the
> type has no sign so sign-extension presumably does not apply? I've tried
> to figure it out from the standard text and I guess fevers and bacterial
> infections are not compatible with reading standardese because I haven't
> worked it out yet :P
> 
> (or maybe you need to be on more serious drugs than I'm on when reading
> it. It might well be that way round!)
> 
>> 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?
> 
> Are there any of those at all? I don't think so. It's possible in theory
> but I don't think we use unsigned int anywhere. I suppose size_t might
> be unsigned int on some platforms, and then this problem might arise.
> 
>> 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.
> 
> I suppose if it was always done *at the return site* so the compiler
> could always see the return type properly, it would always get the cast
> right: my worry is the CTF_ERR at the test site, in the user code we
> cannot affect (except by changing CTF_ERR).

I suppose so, but does it really matter when we cannot do this part? Or 
are you thinking about just having the ctf_set_errno function always 
return -1 (negative one) and let the caller do the automagic conversion? 
If so, I think the idiom with return_value == CTF_ERR is going to fail.

>> 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)?
> 
> Well, you could use a macro to make it less gross. All the code
> duplication makes me wince a bit.

How would a macro work for this?

Consider that a function returns a char*, in this case, neither CTF_ERR, 
nor -1 should be returned, but instead NULL. Using a macro would make 
this problematic to differentiate.

A possibility could be to have a macro like:

#define CTF_SET_ERRNO(fp,err) do { fp->ctf_errno = err; } while(0)

but what's the point?
Is it better to write:

if (some_failure_condition)
   {
     CTF_SET_ERRNO(fp, ENOMEM);
     return -1; // or CTF_ERR, NULL, ..., depending on the function
   }

instead of:

if (some_failure_condition)
   {
     fp->ctf_errno = ENOMEM;
     return -1; // or CTF_ERR, NULL, ..., depending on the function
   }


> But more problematic from my perspective is the implications for the
> callers. We're trying to get either -1 or (some type)-1 to the callers
> so they can error-check it in a consistent and not-too-horrifying
> fashion, after all. (We could add a ctf_is_error() function or macro
> that does whatever magic is needed if it's too baroque, but that would
> be my least favourite option because it looks nothing like a
> conditional.)

Adding a ctf_is_error()-function would be one way to go, but it would be 
an API change while all the others I've been talking about is internal 
into the library without actually touching the interface.

>> 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.
> 
> Agreed, or I'd have proposed it already. Toolchain projects like
> binutils must be conservative, and there are no doubt lots of archaic
> weird but valid targets out there where the bootstrap chain requires
> binutils building before GCC and the system compiler is not C11-capable.
> 
>>> ... 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. :)
> 
> I think we're still narrowing down the problem, really. -1 is such a
> common error return, it's amazing how hard it is to make it work right :)
> 

So, based on all we have talk about so for, I'm leaning towards just 
having the 2 statements in all places where an error should be returned 
and have the change contained inside the library rather than making 
external changes required.
Then the question is, would the CTF_SET_ERRNO macro have anything to 
add? I.e. should we have it even if it's just setting the field or 
should we set the field directly? Regardless, the return statement needs 
to be kept outside that block.

Please let me know what you think so that we can progress on this topic.
I'm currently blocked by this point for the tool chain that I'm supposed 
to deliver.

Kind regards,
Torbjörn