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

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

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

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

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

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

-- 
NULL && (void)