-
-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test failures on s390x #105
Comments
If this were an endian issue, it would be a very interesting manifestation! Looking at the code, this is potentially due to uninitialized memory. But it would be very strange that this isn't occurring in other environments! Are you using the system Python build or a custom one? Any chance you could build Python from source (perhaps via |
This memory should be initialized to NULL, which I thought would be a 0 everywhere. However, #105 reports tests failing and the only explanation I could come up with is that these int fields are never explicitly set to 0. This commit explicitly sets these fields to 0 to possibly fix this issue.
I just pushed a commit that may fix this. Could you please test and see if it does? |
I've just tested it and there's no change in the test results. The same 11 tests are still failing with the same assertion errors. I am using system python. I don't know when I'll have some time to build Python from source, but I'll let you know if running under vanilla Python changes anything here. |
With 0.15.1, there are 24 failures now:
|
In typedef struct {
PyObject_HEAD
...
int closed;
...
} ZstdCompressionWriter; But in static PyMemberDef ZstdCompressionWriter_members[] = {
{"closed", T_BOOL, offsetof(ZstdCompressionWriter, closed), READONLY, NULL},
{NULL}}; This should fix this problem. static PyMemberDef ZstdCompressionWriter_members[] = {
- {"closed", T_BOOL, offsetof(ZstdCompressionWriter, closed), READONLY, NULL},
+ {"closed", T_INT, offsetof(ZstdCompressionWriter, closed), READONLY, NULL},
{NULL}}; Or change the |
Great catch, @animalize . The following patch makes all tests pass on s390x: diff -up zstandard-0.15.1/c-ext/compressionreader.c.orig zstandard-0.15.1/c-ext/compressionreader.c
--- zstandard-0.15.1/c-ext/compressionreader.c.orig 2020-12-31 23:18:06.000000000 +0000
+++ zstandard-0.15.1/c-ext/compressionreader.c 2021-03-01 11:41:09.056060857 +0000
@@ -793,7 +793,7 @@ static PyMethodDef compressionreader_met
{NULL, NULL}};
static PyMemberDef compressionreader_members[] = {
- {"closed", T_BOOL, offsetof(ZstdCompressionReader, closed), READONLY,
+ {"closed", T_INT, offsetof(ZstdCompressionReader, closed), READONLY,
"whether stream is closed"},
{NULL}};
diff -up zstandard-0.15.1/c-ext/decompressionreader.c.orig zstandard-0.15.1/c-ext/decompressionreader.c
--- zstandard-0.15.1/c-ext/decompressionreader.c.orig 2020-12-31 23:18:06.000000000 +0000
+++ zstandard-0.15.1/c-ext/decompressionreader.c 2021-03-01 11:40:35.686060857 +0000
@@ -761,7 +761,7 @@ static PyMethodDef decompressionreader_m
{NULL, NULL}};
static PyMemberDef decompressionreader_members[] = {
- {"closed", T_BOOL, offsetof(ZstdDecompressionReader, closed), READONLY,
+ {"closed", T_INT, offsetof(ZstdDecompressionReader, closed), READONLY,
"whether stream is closed"},
{NULL}};
diff -up zstandard-0.15.1/c-ext/compressionwriter.c.orig zstandard-0.15.1/c-ext/compressionwriter.c
--- zstandard-0.15.1/c-ext/compressionwriter.c.orig 2020-12-31 23:18:06.000000000 +0000
+++ zstandard-0.15.1/c-ext/compressionwriter.c 2021-03-01 11:50:29.546150542 +0000
@@ -323,7 +323,7 @@ static PyMethodDef ZstdCompressionWriter
{NULL, NULL}};
static PyMemberDef ZstdCompressionWriter_members[] = {
- {"closed", T_BOOL, offsetof(ZstdCompressionWriter, closed), READONLY, NULL},
+ {"closed", T_INT, offsetof(ZstdCompressionWriter, closed), READONLY, NULL},
{NULL}};
PyTypeObject ZstdCompressionWriterType = {
diff -up zstandard-0.15.1/c-ext/decompressionwriter.c.orig zstandard-0.15.1/c-ext/decompressionwriter.c
--- zstandard-0.15.1/c-ext/decompressionwriter.c.orig 2020-12-31 23:18:06.000000000 +0000
+++ zstandard-0.15.1/c-ext/decompressionwriter.c 2021-03-01 11:49:31.386150542 +0000
@@ -243,7 +243,7 @@ static PyMethodDef ZstdDecompressionWrit
{NULL, NULL}};
static PyMemberDef ZstdDecompressionWriter_members[] = {
- {"closed", T_BOOL, offsetof(ZstdDecompressionWriter, closed), READONLY,
+ {"closed", T_INT, offsetof(ZstdDecompressionWriter, closed), READONLY,
NULL},
{NULL}};
|
Thanks for verification. After thinking about it, I think change case T_BOOL:
v = PyBool_FromLong(*(char*)addr);
break;
...
case T_INT:
v = PyLong_FromLong(*(int*)addr);
break; |
It gets assigned values 0 and 1, so I guess it doesn't matter much, as long as the type is used consistently everywhere. I think |
Yes, it doesn't matter much in this case. I searched, |
It's up to compiler, this article tested gcc on these machines:
|
We have got same issues on openSUSE Tumbleweed. |
In Zstd(De)?Compression(Reader|Writer) structs, `closed` is declared as `int`: ```C typedef struct { PyObject_HEAD ... int closed; ... } ZstdCompressionWriter; ``` but in [`PyMemberDef`](https://docs.python.org/3/c-api/structures.html#c.PyMemberDef), it's `T_BOOL`: ```C static PyMemberDef ZstdCompressionWriter_members[] = { {"closed", T_BOOL, offsetof(ZstdCompressionWriter, closed), READONLY, NULL}, {NULL}}; ``` which is `char`. This fixes regression tests on s390x (#105). Closes #164.
#164 merged yesterday and fixes the |
You may run tests on s390x with |
FWIW, the Fedora package has been carrying the merged patch since October last year and s390x builds had their test suites pass. |
I have tested the version 0.17.0 in OBS with openSUSE Tumbleweed. I do not receive any error any more for s390x. |
I'm getting 11 failed tests on Fedora 32 on s390x architecture. s390x is big-endian, could this be the reason?
The text was updated successfully, but these errors were encountered: