gh-111140: Adds PyLong_AsNativeBytes and PyLong_FromNative[Unsigned]Bytes functions

[zooba]

• Issue: C API: Consider adding public PyLong_AsByteArray() and PyLong_FromByteArray() functions #111140

[zooba]

Sharing work so far for feedback/concerns.

I like the unsigned/signed semantics (basically, using "unsigned" suppresses an error where you need one more bit to be a sign bit), and I dislike the mess needed to support byteswapping for non-native endian on arbitrary length buffers. But it's written now and seems to work, so I guess it can stay.

The "FromByteArray" prototypes are a lie right now, but they're coming in next :) Probably need a "WithOptions" variant as well, though I have a feeling the implementation can't be any better than _PyLong_FromByteArray so it should be simple.

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @zooba for commit 6e1a89a 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

[zooba]

The only unhappy buildbot seems to be the s390x Fedora one, and that looks unrelated to this (if int conversions in posixmodule were incorrect, I'd expect to see other buildbots fail as well).

Any further feedback?

[zooba]

Thanks @erlend-aasland! Great feedback, and even found a bug (this is why we get reviews!)

I left one open question about doc wording, as I know you're more broadly involved in that than I am.

[encukou]

Shouldn't this return size_t rather than int?

[zooba]

Negative values are required, so we'd have to do Py_ssize_t and not size_t. The cast is just as annoying either way unless we make n_bytes also signed, which is then inconsistent with other APIs (but probably less bad than making it accept int).

We might need some agreed upon guidelines for choosing types for these kinds of purposes. int is a very common return type, and IMHO that makes things easier for people trying to call this from non-C languages, but they probably have no choice but to support Py_ssize_t as a return type too and so it really wouldn't make a huge difference.

I pity the poor CPU that has to convert a 32 billion bit number 🙃

[encukou]

I really need to focus on not mixing up signed/unsigned when I write a comment...

Yes, Py_ssize_t is one of the types that users need to support.
I'd much prefer using Py_ssize_t for sizes. These are arbitrary-sized integers, after all; limited by available memory.

[encukou]

How should I use PyLong_AsNativeBytes if I have an existing signed long I want to fill -- say, as part of a struct?
It seems that I'd need to allocate a buffer with one more byte, and then check that the extra byte is zero. And then memcpy to the destination.

[zooba]

How should I use PyLong_AsNativeBytes if I have an existing signed long I want to fill -- say, as part of a struct?

n = PyLong_AsNativeBytes(v, &st.long_value, sizeof(long), -1);
if (n < 0) {
  // exception raised
} else if (n <= sizeof(long)) {
  // success
} else {
  // value overflows, but st.long_value is set correctly, just truncated
}

What you do in the overflow case is up to you. Maybe you can overallocate? Maybe you just have to fail. But you don't have to overallocate to fit precisely into a signed variable.

For an unsigned variable, you may require one extra bit to store the sign bit, e.g. 2**256-1 requires 257 bits (33 bytes) of storage, and so does -(2**256-1). The extra byte will contain all sign bits in both cases, but in the former we don't need it when we know the value is going to be treated as unsigned (the MSB of the 32-byte value is set in both cases).

There really isn't a consistent way for us to handle this, lacking knowledge of what the value actually is. The current (private) function just fails on all negative values if you ask for unsigned, but that doesn't actually touch this issue at all - it just excludes a whole lot of otherwise valid conversions (e.g. +/-(2**255-1) and smaller always fits in 32 bytes with no loss of data).

So the suggestion there is if you know the magnitude of the value fits into your destination, or you don't care, then ignore positive returns from the function. But again, it only affects positive integers that require precisely the number of bits you've provided when treated as unsigned, and so treating as signed requires one extra.

[encukou]

Thanks.
IMO it's hard to implement a conversion to native unsigned integer on top of this, but, that could be solved by adding PyLong_AsNativeUnsignedBytes. No need to block this PR.

I trust your judgment on int/*size_t as well; no need for that to block the PR.

[zooba]

IMO it's hard to implement a conversion to native unsigned integer on top of this, but, that could be solved by adding PyLong_AsNativeUnsignedBytes. No need to block this PR.

I briefly implemented that. It took the size of the provided buffer, allocated a new one with one extra byte, did exactly the same thing, and copied the result into the original buffer or failed if it wasn't going to fit. It seems wasteful, especially compared to the caller doing a static allocation of a larger buffer themselves and calling this API, but we can't really improve on it that much - calculating that the resulting number is going to be in that one-bit worth of grey area isn't obvious. (And IMHO not offering it makes it more likely that people will implement it efficiently for themselves, rather than complaining that we did it inefficiently.)

The thing that isn't trivial is to reject negative values entirely, which someone may want to do. But that's more likely to be an application requirement than an integration/adapter requirement, so I'd expect they'll have a comparison to zero in their own language before they convert anyway, which means they'll go to a native signed integer rather than directly to unsigned, or they'll do the comparison using a Python comparison.

[zooba]

I trust your judgment on int/*size_t as well; no need for that to block the PR.

@encukou It didn't block, I changed them all to Py_ssize_t 😄 My only concern was that comparing the result to sizeof(...) shouldn't cause a warning, but then I realised that the warning would also occur for (signed) int, so it doesn't matter.

[gpshead]

additional follow on example and test improvements in #115380

[gpshead]

Looking this API over the main thing I don't like is... unrelated to the API. It appears to be doing the right thing twos compliment wise given that it is returning data in whole bytes with the sign extension filling an entire additional byte when necessary as \xff or \x00.

I do find the need for an additional byte if someone is dealing exclusively with a value they already know to be non-negative yet large enough to occupy the high bit "annoying"... but this API isn't primarily for use by people wanting to merely fill a native type like that. They can add a range check of their own and be happy. ie: The prior understanding of the underlying value as discussed above.

Thus my PR making the example not use it to fill in a mere int to not encourage that pattern given native APIs for many types like that exist.

Granted we could change to encouraging this API everywhere in the future instead of people forgetting to if ((value = PyLong_AsLong(x)) == -1 && PyErr_Occurred()) { ... } being a super common flaw in code. This API maybe more readily encourages checking the return value.