Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bytes package tests test implementation defined behaviour #37

Closed
mwhudson opened this issue Aug 19, 2019 · 0 comments · May be fixed by #38
Closed

bytes package tests test implementation defined behaviour #37

mwhudson opened this issue Aug 19, 2019 · 0 comments · May be fixed by #38

Comments

@mwhudson
Copy link

As can currently be seen at https://people.canonical.com/~ubuntu-archive/proposed-migration/update_excuses.html#golang-github-labstack-gommon, the tests for the bytes package fail on arm64, ppc64el and s390x architectures. This turns out to be because the tests depend on the conversion of the (just) too large float math.Pow(2, 64) to int64 returning MinInt64. On these platforms it seems the conversion saturates and returns MaxInt64. FWIW the standard says:

In all non-constant conversions involving floating-point or complex values, if the result type cannot represent the value the conversion succeeds but the result value is implementation-dependent.

so the tests shouldn't depend on this, or if this behaviour is important then the implementation needs to take care to enforce it. I don't really see a reason why the tests need to push the edge of the range of int64 like this so I'll probably patch them in Ubuntu to test smaller values.
ondrejbudai added a commit to ondrejbudai/gommon that referenced this issue Aug 26, 2020
@ondrejbudai
do not rely on undefined behaviour in bytes_test.go
f06a8b0 
8 exbi equals 2^64, therefore it cannot be stored in int64. The tests use
the fact that on x86_64 the following expressions holds true:
int64(0) - 1 == math.MaxInt64.

However, this is not true for other platforms, specifically aarch64, s390x
and ppc64le.

This commit fixes it by testing the library with 7 exbi.

Fixes labstack#37
ondrejbudai added a commit to ondrejbudai/gommon that referenced this issue Aug 26, 2020
@ondrejbudai
do not rely on undefined behaviour in bytes_test.go
ed3a1e5 
8 exbi equals 2^64, therefore it cannot be stored in int64. The tests use
the fact that on x86_64 the following expressions holds true:
int64(0) - 1 == math.MaxInt64.

However, this is not true for other platforms, specifically aarch64, s390x
and ppc64le.

This commit fixes it by testing the library with 7 exbi.

Fixes labstack#37
ondrejbudai added a commit to ondrejbudai/gommon that referenced this issue Aug 26, 2020
@ondrejbudai
do not rely on undefined behaviour in bytes_test.go
2337700 
8 exbi equals 2^64, therefore it cannot be stored in int64. The tests use
the fact that on x86_64 the following expressions holds true:
int64(0) - 1 == math.MaxInt64.

However, this is not true for other platforms, specifically aarch64, s390x
and ppc64le.

This commit fixes it by testing the library with 7 exbi.

Fixes labstack#37
@ondrejbudai ondrejbudai mentioned this issue Aug 26, 2020
Open
ondrejbudai added a commit to ondrejbudai/gommon that referenced this issue Aug 26, 2020
@ondrejbudai
do not rely on undefined behaviour in bytes_test.go
5c8aceb 
8 exbi equals 2^64, therefore it cannot be stored in int64. The tests use
the fact that on x86_64 the following expressions holds true:
int64(0) - 1 == math.MaxInt64.

However, this is not true for other platforms, specifically aarch64, s390x
and ppc64le.

This commit fixes it by testing the library with 7 exbi.

Fixes labstack#37
@aldas aldas closed this as completed Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

do not rely on undefined behaviour in bytes_test.go ondrejbudai/gommon
2 participants
@mwhudson @aldas