bpo-34569: Fix subinterpreter 32-bit ABI, pystate.c/_new_long_object()

[aixtools]

The Lib/test/test__xxsubinterpreters.py test fails in:
ShareableTypeTests.test_int() with the value "-1" when in 32-bit mode (long is 32-bit).

https://bugs.python.org/issue34569

[ericsnowcurrently]

I've left some comments below, but there are a couple of high-level things to address.

First, each PR should correspond to one BPO issue. It looks like this PR has fixes for two issues.

Second, I'd expect the fix to involve changes to _long_shared() and/or _new_long_object() (in Python/pystate.c) or even PyLong_AsLongLong()/PyLong_FromLongLong/ULONG_MAX. That said, I'm no expert in this platform-specific C-level stuff, so it may be worth getting feedback from someone else too. :)

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[aixtools]

I have made the requested changes; please review again.

[bedevere-bot]

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

[ericsnowcurrently]

Thanks for making those changes. I'm happy with the code.

That said, as implied above I would expect the test to pass as-is. -1 is a legitimate value to pass through a channel. The fact that the test is failing means that it's doing its job: letting us know when there's a bug. :) Most likely, either the code in Python/pystate.c (_long_shared() and/or _new_long_object()) or the implementation of PyLong_AsLongLong()/PyLong_FromLongLong()/ULONG_MAX needs to be fixed.

Consequently, this PR should probably not be merged. If there's any urgency to getting a passing test suite then I'd recommend making a PR to skip the test under AIX (as a temporary stopgap). Either way, the BPO issue should stay open until the underlying bug is resolved.

[aixtools]

On 19/09/2018 17:53, Eric Snow wrote: Thanks for making those changes. I'm happy with the code. That said, as implied above I would expect the test to pass as-is. `-1` is a legitimate value to pass through a channel. The fact that the test is failing means that it's doing its job: letting us know when there's a bug. :) Most likely, either the code in Python/pystate.c (`_long_shared()` and/or `_new_long_object()`) or the implementation of `PyLong_AsLongLong()`/`PyLong_FromLongLong()`/`ULONG_MAX` needs to be fixed. Consequently, this PR should probably not be merged. If there's any urgency to getting a passing test suite then I'd recommend making a PR to skip the test under AIX (as a temporary stopgap). Either way, the BPO issue should stay open until the underlying bug is resolved.

Understood. Would it "work" to rename to a different issue number (29970 is an issue already opened for 'test' passing. As to making it work properly, I am not comfortable enough with internals to actually make a change - but I will make the time later to find the line where it goes wrong (basically where the value it is passed is assigned to the "void". The 'solution' will be something along the line of #if 32-bit do something that typecasts from 32-bit to 64-bit. I do not have enough experience/knowledge to know if type casting from 32-bit "int" to 64-bit "int" will work in all cases. As to the test, yes, -1 is a valid value. -2 would fail - rather any negative value - would fail in the same way. So, as to my question above - if that is okay, should all the other tests be passing then I'll change this issue number to 29970. But if I (better we) have made progress with a real solution we can merge those changes instead (and revert the test back to original). Michael

[aixtools]

afaik - there is a red X because there were issues with the Linux-PR systems. Is there a recognized way to rerun the tests?

[aixtools]

@ericsnowcurrently I have made the requested changes; please review again.

Now a change is pystate.c/new_long_object()

[bedevere-bot]

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

[aixtools]

@ericsnowcurrently - Hi! Have you had time to check this is what you intended for me to do?

[kadler]

@ericsnowcurrently This seems to be a sign extension bug.

_PyCrossInterpreterData::data is a void* which is holding the hex value 0xFFFFFFFF. As a signed integer it is -1, but as an unsigned integer it is 4294967295. PyLong_FromLongLong takes a long long, so the compiler will implicitly cast void* to long long. On 32-bit systems, this will mean that the size of the type will be increased and that means that the signedness of the types will come in to play in determining sign extension.

It seems that on this platform/compiler, void* is treated as an unsigned int, so there is no sign extension done when casting to a long long, eg. 0xFFFFFFFF -> 0x00000000FFFFFFFF. Casting first to a signed integer of the same size as a pointer will fix this. You can validate with this sample program:

#include <stdio.h>

int main()
{
   printf("%lld\n", (long long)(void*)-1);
   return 0;
}

With GCC:

-bash-4.3$ gcc -maix32 testi.c && ./a.out
-1

With XLC:

-bash-4.3$ xlc -q32 testi.c && ./a.out 
4294967295

The code as-is will work, but intptr_t can be used for this instead of the #if condition to keep the code cleaner.

[kadler]

There should probably also be a comment added to state why PyLong_FromVoidPtr was not used, since that would seem most appropriate. The problem is that PyLong_FromVoidPtr explicitly forces the pointer to be unsigned, so you'd never get -1.

[ncoghlan]

The actual code change here now looks good to me, but I'm not sure where the NEWS entry should go given that the cross-interpreter data channels aren't officially a public API yet.

@ericsnowcurrently perhaps leaving the entry in Tests makes the most sense right now, even though the code is actually part of the builtins?

[ericsnowcurrently]

tl;dr I'll merge this Wednesday Jan 9 (or by the 11th of the latest), unless someone tells me that we could lose info by using intptr_t.

Thanks, all, for diving into this. The solution mostly looks good to me. Further, my experience with C intrinsic types at the compiler level is small so I'm willing to defer to Nick's judgement here. :)

My only concern is that types match up so there is no loss of information. In the code in pystate.c, the conversions follow this flow:

pack:

• PyLong_AsLongLong(): PyLongObject -> long long
• cast (implicit): long long -> int64_t
• cast: int64_t -> void *

unpack:

• cast: void * -> int64_t
• cast (implicit): int64_t -> long long
• PyLong_FromLongLong(): long long -> PyLongObject

The code relies on the equivalency of int64_t, long long, and void *. It sounds like the equivalency is not perfect on all platforms. The suggested solution further relies on the equivalency of intptr_t and the others. As long as it lines up I'm okay with it. The key guarantee is that sizeof(long long) <= sizeof(void *) and that any intermediate types are at least as large as void *. If that's true of intptr_t (and it should be) then we're good here and we can merge this change. :)

If I don't here any objections by next Wednesday then I'll merge it then.

[kadler]

According to the C99 spec which introduced long long, it must be at least 64-bits in size. (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf section 5.2.4.2.1). So unless you were running on a system where the pointers are greater than 64-bit (unlikely), this should fine.