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

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

This is a really fiddly horror show, isn't it...

On 3 Oct 2023, Torbjorn SVENSSON outgrape:

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

Oh. That needs fixing then -- there's no other way for the caller to
tell what to test, and honestly even '-1 for signed, CTF_ERR for
unsigned and ctf_id_t, == NULL for pointers' is complex enough that it's
a source of errors all on its own :(

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

I was assuming that a *macro*

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

would return the right thing automatically -- but oh ugh that's a
behaviour change, ctf_set_errno does not do a return currently. That
won't work. Fixing that to get a ctf_set_errno macro to return a
suitable value requires the statement-expression extension which of
course we cannot rely on being present in binutils :( so a
ctf_set_errno macro is untenable. Dammit.

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

I was assuming that the callee would do it.

>             If so, I think the idiom with return_value == CTF_ERR is
> going to fail.

Oh wait, I see -- if some return types are wider than unsigned long,
CTF_ERR is not going to equal -1 for comparisons against that type. Ew.
And of course what that type is is platform-dependent.

Maybe we *can* get away with using uintmax_t in the definition of
CTF_ERR?

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

Not sure. I'm casting about and not even writing enough test cases I'm
afraid. :(

I disproved the macro in this reply anyway (see above). Sorry, I'm
thinking very slowly right now.

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

Ah yes, pointers are the third case. Obviously NULL == error for those.

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

I'd say stick with the fp->ctf_errno assignment, it's far clearer than a
SHOUTY MACRO that just does an assignment. The whole point of
ctf_set_errno() in the first place was to avoid the need for multiple
statements and thus a new block: if we need multiple statements anyway
because of return-type pain like this, we might as well dump the macro.

(assuming this works, because that's a lot of changes to stick you
with.)

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

That's exactly my objection too (also, it's clunky as hell). I really do
want to avoid that. My big fear here is that you've found some
fundamental problem with a -1 signed, CTF_ERR unsigned return (we can't
even change the definition of CTF_ERR, because it's baked into callers,
while libctf can change under them.)

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

I think you're right.

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

I'd go for dumping CTF_SET_ERRNO: just switch to a scheme where we set
fp->ctf_errno and do a return. It adds a few more {} blocks but that's
unavoidable in the absence of statement expressions. (And half the
places we care about don't need new blocks anyway.)

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

Oh I'm sorry!

-- 
NULL && (void)