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.

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

host-gcc: exclude -lgcc to fix -mx32 [qemu_]x86_64 regression #12805

Merged
merged 1 commit into from
Jan 31, 2019

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Jan 29, 2019

PR #9522 series ending with commit c2c9265 ("tests: cmsis: Disable
two cmsis portability tests on x86_64") added -mx32 support for the
x86_64 ARCH and qemu_x86_64. While this was implemented in
"compiler/gcc/target.cmake" as fall back from cross-compilation to the
host compiler, it worked with a direct ZEPHYR_TOOLCHAIN_VARIANT=host
too.

Later, -lgcc was added to "compiler/host-gcc/target.cmake" by PR #12674
to fix the -m32 x86 build. This broke the x86_64 build when using
ZEPHYR_TOOLCHAIN_VARIANT=host because even "multilib" packages usually
don't feature the -mx32 version of libgcc.

Fix this by excluding -lgcc in compiler/host-gcc/target.cmake just like
compiler/gcc/target.cmake always did for x86_64.

Signed-off-by: Marc Herbert [email protected]

@codecov-io
Copy link

codecov-io commented Jan 29, 2019

Codecov Report

Merging #12805 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #12805   +/-   ##
=======================================
  Coverage   53.51%   53.51%           
=======================================
  Files         239      239           
  Lines       26989    26989           
  Branches     6521     6521           
=======================================
  Hits        14442    14442           
  Misses       9881     9881           
  Partials     2666     2666

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d210dc...3a93cb8. Read the comment docs.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 29, 2019

While this was implemented in "compiler/gcc/target.cmake" as fall back from cross-compilation to the host compiler, it worked with a direct ZEPHYR_TOOLCHAIN_VARIANT=host too.

I'm quite confused about which toolchain configuration(s) x86_64 was originally tested with. This is why I didn't test x86_64 with ZEPHYR_TOOLCHAIN_VARIANT=host until after a chat with @andyross and let this regression slip. More details just added to PR #9522

@nashif would it be a lot of CI work to add a minimal "make run" / "ninja run" for all BOARD=qemu_* and how could I/we help?

Copy link
Collaborator

@mped-oticon mped-oticon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I think x32 can be safely be ignored - esp when Linux have been talking abourt dropping support.

Finally, note that host-gcc and native_posix are useful beyond Intel architectures.
(E.g. I am running native_posix on MIPS and MIPSel.)

# -march={pentium,lakemont,...} do not automagically imply -m32, so
# adding it here.

# There's only one 64bits ARCH (actually: -mx32). Let's exclude it to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Until arm aarch64 is added :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... but not MIPS64? :-)

This is actually why I'm suggesting in the next paragraph to move this -m32 flag entirely out of this file in the longer term and spread them instead next to the corresponding -march flags. @SebastianBoe ?
This would be a significant refactoring though, way beyond the scope of this small regression fix.

# -march={pentium,lakemont,...} do not automagically imply -m32, so
# adding it here.

# There's only one 64bits ARCH (actually: -mx32). Let's exclude it to
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By x32 you mean the x32 ABI which has more registers than i386, right?
https://en.wikipedia.org/wiki/X32_ABI

But native_posix requires 32-bit always (so should be i386 compatible (and x32 I suppose)) and then you want to not set -m32 if we are on x86-64? What am I missing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes native_posix is neither 64 bits nor x32, which is why it's unrelated to this fix.

then you want to not set -m32 if we are on x86-64?

When compiling x86_64 with ZEPHYR_TOOLCHAIN_VARIANT=host right now, -m32 is (silently) overridden by -mx32 because -mx32 (luckily?) comes last. That doesn't seem right. This first part of the patch doesn't fix anything but it removes this element of luck.

@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 30, 2019

btw, I think x32 can be safely be ignored -

x32 = qemu_x86_64 is what this fixes...
Check the references to @andyross ' work above for more details.

esp when Linux have been talking abourt dropping support.

Well this is not Linux, is it? :-)

(E.g. I am running native_posix on MIPS and MIPSel.)

Thanks! I was actually wondering about something like that. Are you adding it to the Zephyr CI? :-)

@andyross
Copy link
Contributor

FWIW: -mx32 is an interrim solution for x86_64 that absolved me of the need to find and fix word size bugs in all of Zephyr. We don't care about anything but toolchain support for it, and gcc/binutils will continue to support it. The core ("xuk") of the x86_64 layer already works with native 64 bit code, so it's not like there are any huge roadblocks. Just need to turn it on and start fixing bugs. Maybe for 1.15.

Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks correct to me modulo my limited abilities with cmake.

And I agree that we probably want some level of validation somewhere of ZEPHYR_TOOLCHAIN_VARIANT=host. It's the kind of thing that can bitrot quickly. Maybe throw one board into the slow CI runs that uses this and define our supported "host" as Ubuntu Xenial or whatever.

PR zephyrproject-rtos#9522 series ending with commit c2c9265 ("tests: cmsis: Disable
two cmsis portability tests on x86_64") added -mx32 support for the
x86_64 ARCH and qemu_x86_64. While this was implemented in
"compiler/gcc/target.cmake" as fall back from cross-compilation to the
host compiler, it worked with a direct ZEPHYR_TOOLCHAIN_VARIANT=host
too.

Later, -lgcc was added to "compiler/host-gcc/target.cmake" by PR zephyrproject-rtos#12674
to fix the -m32 x86 build. This broke the x86_64 build when using
ZEPHYR_TOOLCHAIN_VARIANT=host because even "multilib" packages usually
don't feature the -mx32 version of libgcc.

Fix this by excluding -lgcc in compiler/host-gcc/target.cmake just like
compiler/gcc/target.cmake always did for x86_64.

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb
Copy link
Collaborator Author

marc-hb commented Jan 31, 2019

Pure rebase noise, zero code change.

@nashif
Copy link
Member

nashif commented Jan 31, 2019

@marc-hb for the CI enhancements, open an issue/enhancement so we can track it

@nashif
Copy link
Member

nashif commented Jan 31, 2019

btw, the way I test this:

export ZEPHYR_TOOLCHAIN_VARIANT=host
sanitycheck -p qemu_x86_64 --force-toolchain

@nashif nashif merged commit e526a2b into zephyrproject-rtos:master Jan 31, 2019
@marc-hb marc-hb deleted the m32 branch February 20, 2019 00:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants