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

zassert_equal fails for float comparisons on native_posix #61345

Closed
martinjaeger opened this issue Aug 9, 2023 · 6 comments · Fixed by #61363
Closed

zassert_equal fails for float comparisons on native_posix #61345

martinjaeger opened this issue Aug 9, 2023 · 6 comments · Fixed by #61363
Assignees
@aescolar
Labels
area: native port Host native arch port (native_sim) bug The issue is a bug, or the PR is fixing a bug

Comments

@martinjaeger
Copy link
Member

martinjaeger commented Aug 9, 2023 •
edited by keith-packard
Loading

Describe the bug

Below code snippet fails if run on native_posix:

float f = 4.56F;
zassert_equal(f, 4.56F); /* evaluates to false */

To Reproduce

Steps to reproduce the behavior:

  1. Copy above snippet into a test, e.g. tests/ztest/base/src/main.c
  2. Run west build -b native_posix tests/ztest/base -t run
  3. See that the test fails

Expected behavior

The variable and the floating constant should be equal.

Impact

Hard to reproduce and unexpected bugs (in unit tests or even somewhere else).

Logs and console output

GCC options:

$ ninja -v
[1/1] : && ccache /usr/bin/gcc  -gdwarf-4 zephyr/CMakeFiles/zephyr_pre0.dir/misc/empty_file.c.obj -o zephyr/zephyr.elf  zephyr/CMakeFiles/offsets.dir/./arch/posix/core/offsets/offsets.c.obj  -fuse-ld=bfd  -T  zephyr/linker_zephyr_pre0.cmd  -Wl,-Map=/home/martin/XXX/zephyr/build/zephyr/zephyr_pre0.map  -Wl,--whole-archive  app/libapp.a  zephyr/libzephyr.a  zephyr/arch/arch/posix/core/libarch__posix__core.a  zephyr/soc/posix/inf_clock/libsoc__posix__inf_clock.a  zephyr/boards/posix/native_posix/libboards__posix__native_posix.a  zephyr/subsys/testsuite/ztest/libsubsys__testsuite__ztest.a  zephyr/drivers/console/libdrivers__console.a  zephyr/drivers/timer/libdrivers__timer.a  -Wl,--no-whole-archive  zephyr/kernel/libkernel.a  -L/home/martin/XXX/zephyr/build/zephyr  -Wl,--gc-sections  -Wl,--build-id=none  -Wl,--sort-common=descending  -Wl,--sort-section=alignment  -Wl,-u,_OffsetAbsSyms  -Wl,-u,_ConfigAbsSyms  -Wl,-no-pie  -m32  -ldl  -pthread  -lm && cd /home/martin/XXX/zephyr/build/zephyr && /usr/bin/cmake -E true && cd /home/martin/XXX/zephyr/build/zephyr && /usr/bin/cmake -E copy zephyr_pre0.map zephyr.map && /usr/bin/readelf -e zephyr.elf > zephyr.stat && /usr/bin/cmake -E copy zephyr.elf zephyr.exe

Environment:

  • OS: Manjaro Linux on x86_64 (Intel Core i5 Gen 8)
  • Toolchain: gcc-Version 12.2.1 20230201 (GCC)
  • Commit SHA: b40c052

Additional context

After investigation by @keith-packard the old 8087 FPU seems to be the root cause of the issue. Compiler flags -msse -mfpmath=sse or -std=gnu11 fix the issue.

We need to decide which compiler flags we should use for native_posix in Zephyr.

Related Discord discussion:
https://discord.com/channels/720317445772017664/883404732423606303/1138773080198627408

Related GCC bug / discussion:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92875

CC @aescolar @stephanosio @nashif
@martinjaeger martinjaeger added the bug The issue is a bug, or the PR is fixing a bug label Aug 9, 2023
@keith-packard
Copy link
Collaborator

Given the GCC discussion linked above, it seems clear that this wouldn't be considered a bug and so is unlikely to get changed.

If possible, switching to -msse -mfpmath=sse would both fix this issue as well as offer true 32/64 bit float computations so that results would match the usual floating point formats used in every other machine.

aescolar added a commit to aescolar/zephyr that referenced this issue Aug 10, 2023
@aescolar
POSIX arch: Fix literal floating comparison in 32bit targets
637ea00 
When building the 32bit native board targets variants
for x86(-64) hosts, gcc will promote float literals to double
(See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92875 )

This can result in unexpected comparison differences.

This is due to the compiler using the 8087 float mode by
default.
Instead let's tell the compiler to use the SSE float path,
which is the default for 64 bit x86-64 builds.

The assumption that any x86 host used for development
will have SSE support should be safe enough.

For more background see
zephyrproject-rtos#61345

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
@aescolar aescolar mentioned this issue Aug 10, 2023
Merged
@aescolar
Copy link
Member

aescolar commented Aug 10, 2023 •
edited
Loading

@keith-packard @martinjaeger Thanks for the very detailed analysis :)
I just posted a PR with one of the fixes proposed here (I lean much more towards the switch for enabling the sse float, than towards switching the language mode)

@aescolar aescolar added the area: native port Host native arch port (native_sim) label Aug 10, 2023
@aescolar aescolar self-assigned this Aug 10, 2023
@keith-packard
Copy link
Collaborator

@keith-packard @martinjaeger Thanks for the very detailed analysis :) I just posted a PR with one of the fixes proposed here (I lean much more towards the switch for enabling the sse float, than towards switching the language mode)

I agree -- it makes the native_posix port work more like other environments and improves performance. Probably need the TCS to read on that?

@aescolar
Copy link
Member

aescolar commented Aug 10, 2023 •
edited
Loading

Probably need the TCS to read on that?

@keith-packard You think any of this will be controversial?

@keith-packard
Copy link
Collaborator

Probably need the TCS to read on that?

@keith-packard You think any of this will be controversial?

It changes the base platform requirements for the native_posix target so that it only works on targets with sse2 (see comment indicating that we need -msse2 instead of -msse). That's only been around since 2004 :-)

aescolar added a commit to aescolar/zephyr that referenced this issue Aug 11, 2023
@aescolar
POSIX arch: Fix literal floating comparison in 32bit targets
cd2ca27 
When building the 32bit native board targets variants
for x86(-64) hosts, gcc will promote float literals to double
(See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92875 )

This can result in unexpected comparison differences.

This is due to the compiler using the 8087 float mode by
default.
Instead let's tell the compiler to use the SSE float path,
which is the default for 64 bit x86-64 builds.

The assumption that any x86 host used for development
will have SSE support should be safe enough.

For more background see
zephyrproject-rtos#61345

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
@aescolar
Copy link
Member

aescolar commented Aug 12, 2023 •
edited
Loading

That's only been around since 2004 :-)

@keith-packard Given that the native boards are development/test targets, I think that is ok. If some developer finds it a problem, they can file an issue and is so we add a kconfig choice to disable this switch.

fabiobaltieri pushed a commit that referenced this issue Aug 14, 2023
@aescolar @fabiobaltieri
POSIX arch: Fix literal floating comparison in 32bit targets
d4e48d5 
When building the 32bit native board targets variants
for x86(-64) hosts, gcc will promote float literals to double
(See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92875 )

This can result in unexpected comparison differences.

This is due to the compiler using the 8087 float mode by
default.
Instead let's tell the compiler to use the SSE float path,
which is the default for 64 bit x86-64 builds.

The assumption that any x86 host used for development
will have SSE support should be safe enough.

For more background see
#61345

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
aescolar added a commit to aescolar/zephyr that referenced this issue Aug 14, 2023
@aescolar
POSIX arch: Fix literal floating comparison in 32bit targets
f367e71 
When building the 32bit native board targets variants
for x86(-64) hosts, gcc will promote float literals to double
(See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92875 )

This can result in unexpected comparison differences.

This is due to the compiler using the 8087 float mode by
default.
Instead let's tell the compiler to use the SSE float path,
which is the default for 64 bit x86-64 builds.

The assumption that any x86 host used for development
will have SSE support should be safe enough.

For more background see
zephyrproject-rtos#61345

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
(cherry picked from commit d4e48d5)
@aescolar aescolar mentioned this issue Aug 14, 2023
Merged
umarnisar92 pushed a commit to nisarumar/zephyr that referenced this issue Aug 15, 2023
@aescolar
POSIX arch: Fix literal floating comparison in 32bit targets
290d1c8 
When building the 32bit native board targets variants
for x86(-64) hosts, gcc will promote float literals to double
(See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92875 )

This can result in unexpected comparison differences.

This is due to the compiler using the 8087 float mode by
default.
Instead let's tell the compiler to use the SSE float path,
which is the default for 64 bit x86-64 builds.

The assumption that any x86 host used for development
will have SSE support should be safe enough.

For more background see
zephyrproject-rtos#61345

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
coreboot-org-bot pushed a commit to coreboot/zephyr-cros that referenced this issue Aug 15, 2023
@aescolar
POSIX arch: Fix literal floating comparison in 32bit targets
b784a5d 
When building the 32bit native board targets variants
for x86(-64) hosts, gcc will promote float literals to double
(See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92875 )

This can result in unexpected comparison differences.

This is due to the compiler using the 8087 float mode by
default.
Instead let's tell the compiler to use the SSE float path,
which is the default for 64 bit x86-64 builds.

The assumption that any x86 host used for development
will have SSE support should be safe enough.

For more background see
zephyrproject-rtos/zephyr#61345

(cherry picked from commit d4e48d5)

Original-Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
GitOrigin-RevId: d4e48d5
Change-Id: I7a20c74f4bbcabbf98d56be670417aa8b882b63c
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/4780807
Commit-Queue: Al Semjonovs <asemjonovs@google.com>
Reviewed-by: Al Semjonovs <asemjonovs@google.com>
Tested-by: Al Semjonovs <asemjonovs@google.com>
Tested-by: ChromeOS Prod (Robot) <chromeos-ci-prod@chromeos-bot.iam.gserviceaccount.com>
meshium pushed a commit to meshium/zephyr that referenced this issue Aug 16, 2023
@aescolar @meshium
POSIX arch: Fix literal floating comparison in 32bit targets
07adca3 
When building the 32bit native board targets variants
for x86(-64) hosts, gcc will promote float literals to double
(See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92875 )

This can result in unexpected comparison differences.

This is due to the compiler using the 8087 float mode by
default.
Instead let's tell the compiler to use the SSE float path,
which is the default for 64 bit x86-64 builds.

The assumption that any x86 host used for development
will have SSE support should be safe enough.

For more background see
zephyrproject-rtos#61345

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
npal-cy pushed a commit to npal-cy/zephyr-1 that referenced this issue Oct 3, 2023
@aescolar @npal-cy
POSIX arch: Fix literal floating comparison in 32bit targets
238a75f 
When building the 32bit native board targets variants
for x86(-64) hosts, gcc will promote float literals to double
(See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92875 )

This can result in unexpected comparison differences.

This is due to the compiler using the 8087 float mode by
default.
Instead let's tell the compiler to use the SSE float path,
which is the default for 64 bit x86-64 builds.

The assumption that any x86 host used for development
will have SSE support should be safe enough.

For more background see
zephyrproject-rtos#61345

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
jgl-meta pushed a commit that referenced this issue Oct 5, 2023
@aescolar @jgl-meta
POSIX arch: Fix literal floating comparison in 32bit targets
a287c88 
When building the 32bit native board targets variants
for x86(-64) hosts, gcc will promote float literals to double
(See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92875 )

This can result in unexpected comparison differences.

This is due to the compiler using the 8087 float mode by
default.
Instead let's tell the compiler to use the SSE float path,
which is the default for 64 bit x86-64 builds.

The assumption that any x86 host used for development
will have SSE support should be safe enough.

For more background see
#61345

Signed-off-by: Alberto Escolar Piedras <alberto.escolar.piedras@nordicsemi.no>
(cherry picked from commit d4e48d5)
@martinjaeger martinjaeger mentioned this issue Jan 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aescolar aescolar

Labels
area: native port Host native arch port (native_sim) bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

POSIX arch: Fix literal floating comparison in 32bit targets aescolar/zephyr
3 participants
@keith-packard @martinjaeger @aescolar