compressVerify fails on ARM #713
Comments
|
@arkellr do you have an opinion on this None of this effects the core operation of the library itself: the data was being compressed identically, it's only the test itself that was erroneously reporting an issue |
|
That PR change looks reasonable.
The explicit “unsigned char” cast should ensure predictable behavior across archs.
Yes the intent was that checksum routine (i grabbed it from Knuths book) produce the same chksum across archs.
… On May 9, 2020, at 19:02, peterhillman ***@***.***> wrote:
@arkellr do you have an opinion on this
PR #717 has a proposed fix. The issue appears to be that on the ARM, 8 bit signed values are not sign-extended when converting to 32 bit unsigned, but on x86 that does happen. The change should force all architectures to work the same way as the ARM, which might have been what is intended. The precomputed checksums have been updated to the new values.
It might also be possible to update the code so the ARM works the same way as the x86, but I don't know the best approach.
None of this effects the core operation of the library itself: the data was being compressed identically, it's only the test itself that was erroneously reporting an issue
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
btw... ‘cptr’ could be ‘const unsigned char *’ ie.
const unsigned char* cptr = reinterpret_cast<const unsigned char*>(uncompressed);
... which saves on hvin to cast in the loop?
… On May 9, 2020, at 19:46, Arkell Rasiah ***@***.***> wrote:
That PR change looks reasonable.
The explicit “unsigned char” cast should ensure predictable behavior across archs.
Yes the intent was that checksum routine (i grabbed it from Knuths book) produce the same chksum across archs.
>> On May 9, 2020, at 19:02, peterhillman ***@***.***> wrote:
>>
>
> @arkellr do you have an opinion on this
> PR #717 has a proposed fix. The issue appears to be that on the ARM, 8 bit signed values are not sign-extended when converting to 32 bit unsigned, but on x86 that does happen. The change should force all architectures to work the same way as the ARM, which might have been what is intended. The precomputed checksums have been updated to the new values.
> It might also be possible to update the code so the ARM works the same way as the x86, but I don't know the best approach.
>
> None of this effects the core operation of the library itself: the data was being compressed identically, it's only the test itself that was erroneously reporting an issue
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
I've updated the PR as suggested. It's slightly messier because it needs to go through the user-defined conversion |
|
awesome... thx for figuring it out.
btw not sure how feasible... but might be good if the jenkins ci did an ARM pass at some point.
… On May 9, 2020, at 20:35, peterhillman ***@***.***> wrote:
I've updated the PR as suggested. It's slightly messier because it needs to go through the user-defined conversion operator() from Array to const char* first.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
It would be good to have a CI test for ARM, though it took my raspberry pi more than five hours to run the full IlmImfTest, so it might not be possible to do that very often. OpenEXR is built for a lot of different processors and OSes, more than could be supported by a single CI. Perhaps folks who regularly build for more unusual systems do frequent build and tests of the master branch between releases. Flagging up any issues caused by the latest commits would mean they could be addressed before the next release. |
…um (AcademySoftwareFoundation#717) * cast to unsigned in testHuf checksum to avoid architectures sign extending differently Signed-off-by: Peter Hillman <peterh@wetafx.co.nz>
…um (AcademySoftwareFoundation#717) * cast to unsigned in testHuf checksum to avoid architectures sign extending differently Signed-off-by: Peter Hillman <peterh@wetafx.co.nz> Signed-off-by: smartin-13 <61707536+smartin-13@users.noreply.github.com>
compressVerify from testHuf.cpp fails on ARMv7 and AAarch64:
The text was updated successfully, but these errors were encountered: