Fix Bug #75284 sha3 is not supported on bigendian machine#2789
Fix Bug #75284 sha3 is not supported on bigendian machine#2789remicollet wants to merge 1 commit intophp:masterfrom
Conversation
remicollet
force-pushed
the
issue-75284
branch
from
fc3141f to
b10cf6a
Compare
|
Fedora scratch build with this patch applied (+ 7c83579) Ok, on bigendian Ok on little endian (no warning) |
|
Will probably do the opposite later
P.S. or not... so merge-up from 7.1 will fail and raise dev attention about different implementation... |
|
@remicollet, did you try the config.m4 only diff I posted to the bug report? I think we can fallback on the provided 32bit algo when we encounter a 64bit BE machine. |
|
@sgolemon thanks for digging on this Yes, your proposal seems much more simpler, but using 32 bits code on 64 bits... seems strange... and indeed doesn't work: From Test build https://koji.fedoraproject.org/koji/taskinfo?taskID=22202631 FAILED TEST SUMMARYhash_copy() via clone [ext/hash/tests/hash-clone.phpt] TEST FAILURE: ../ext/hash/tests/hash-clone.diff -- Well... switching to old 7.1 slow algo is not better.... (perhaps even worst) So I really need to book a bigendian server to try to fix this issue in 7.0+ and then come back to this 7.2+ one... |
|
Ok, 1 step done, slow 7.1 algo fixed in fa78afa |
remicollet
force-pushed
the
issue-75284
branch
2 times, most recently
from
4af3668 to
95a5a08
Compare
remicollet
force-pushed
the
issue-75284
branch
from
95a5a08 to
d3ff2d3
Compare
|
Patch simplified, CI ok. @sgolemon Any other good idea ? |
|
@remicollet Damn, that's unfortunate, I was hoping the 32bit path would just be less efficient. Definitely wanted someone with hardware to try though, cause the above was a easily a possible outcome. My only other "good idea" is to fix the library upstream. Let me see if I can pull that off (I actually can get a 64bit BE machine, just not on short notice) and if we get an upstream fix in place then we can have a later discussion as to whether or not we should pull it back onto 7.2 |
| EXT_HASH_SHA3_SOURCES="$SHA3_OPT_SRC $SHA3_DIR/KeccakHash.c $SHA3_DIR/KeccakSponge.c" | ||
| PHP_HASH_CFLAGS="-I@ext_srcdir@/$SHA3_DIR -DKeccakP200_excluded -DKeccakP400_excluded -DKeccakP800_excluded" | ||
|
|
||
| if test $ac_cv_c_bigendian_php = yes; then |
There was a problem hiding this comment.
This over-pessimizes big endian. 32bit BE works fine on 32bitBE archs (well, I assume it does based on scanning the code). BE is a minority regardless, so it's probably fine, but I'd be inclined to only go slow route for the broken path.
There was a problem hiding this comment.
I only have 64-bit bigendian target.... :(
There was a problem hiding this comment.
@sgolemon I think the safer way for now if to apply this PR (at least before RC4, so I can build 7.2 in Fedora), then we can work on it later... but I indeed think bigendian is minority, and 32-bit another minority... 1% of 1%... (we still have some for RHEL-6... 7 years old... and 7.2 already mostly unsupported there)
There was a problem hiding this comment.
couldnt drop 32bit support be a great thing for php 7.3/8.0 (for the whole php-src codebase)?
There was a problem hiding this comment.
@staabm I don't think we can drop 32-bit support, mostly because of ARM computers which are still quite common (aarch64 is not yet widely used)
|
Merged |
3 participants
This PR restore the old slow algo, which will be only used on bigendian computer.