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

cmake: clang: Support host's clang for non-MCU x86 targets on Linux #14077

Merged
merged 14 commits into from
Apr 30, 2019

Conversation

ozhuraki
Copy link
Collaborator

@ozhuraki ozhuraki commented Mar 5, 2019

Pick host's clang if ZEPHYR_TOOLCHAIN_VARIANT=llvm.

This has been tested on samples/net/sockets/echo native_posix application with clang-7 and upstream’s version.

Compiles, links and runs with and without optimizations.

Instructions:

# sudo apt-get install clang
# cd samples/net/sockets/echo
# export ZEPHYR_TOOLCHAIN_VARIANT=llvm
# mkdir build
# cd build
# cmake -D BOARD=native_posix ..

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

doc change LGTM...

@codecov-io
Copy link

codecov-io commented Mar 5, 2019

Codecov Report

Merging #14077 into master will increase coverage by 0.65%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14077      +/-   ##
==========================================
+ Coverage   52.02%   52.68%   +0.65%     
==========================================
  Files         307      307              
  Lines       45484    45473      -11     
  Branches    10535    10530       -5     
==========================================
+ Hits        23662    23956     +294     
+ Misses      17030    16640     -390     
- Partials     4792     4877      +85
Impacted Files Coverage Δ
subsys/testsuite/ztest/include/ztest_assert.h 38.88% <0%> (-38.89%) ⬇️
subsys/testsuite/ztest/src/ztest.c 65.28% <0%> (-7.44%) ⬇️
include/net/net_pkt.h 88.62% <0%> (-0.38%) ⬇️
include/can.h 100% <0%> (ø) ⬆️
subsys/net/l2/ethernet/ethernet.c 53.07% <0%> (ø) ⬆️
include/kernel.h 94.52% <0%> (ø) ⬆️
kernel/thread.c 84.61% <0%> (ø) ⬆️
include/net/net_context.h 78.18% <0%> (ø) ⬆️
drivers/ethernet/eth_native_posix.c 30.3% <0%> (+0.71%) ⬆️
subsys/net/ip/utils.c 77.65% <0%> (+0.83%) ⬆️
... and 7 more

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 7967829...d5b8d99. Read the comment docs.


find_program(CMAKE_C_COMPILER clang )
#find_program(CMAKE_CXX_COMPILER clang++ ) # Not used currently
#find_program(CMAKE_OBJCOPY objcopy ) # Not used currently
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure about that? I know a bunch of x86 post build steps use objcopy.
I'd rather we not commit commented-out code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andrewboie

I just hecked for native_posix, it’s used at CMake configure/generate step, but not at compile step.

Sure, let’s include it.

-m32
-MMD
-MP
${ARCH_FLAG}
-include ${ZEPHYR_BASE}/arch/posix/include/posix_cheats.h
)

if(NOT CMAKE_C_COMPILER_ID STREQUAL "Clang") # clang errors on -fno-freestanding
zephyr_compile_options(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can and should avoid this if statement by doing zephyr_cc_options() instead of zephyr_compile_options.

zephyr_compile_definitions(
_FORTIFY_SOURCE=2
)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't sane. We are doing "if clang" in a gcc file. Clang should instead not use gcc's target_security_fortify.cmake.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@SebastianBoe

You are right. I checked, clang doesn't understand fortify at all. I refined it, please take a look.

@SebastianBoe
Copy link
Collaborator

Would it be possible/desirable to have clang and llvm support host toolchains instead of introducing host-clang and host-llvm?

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.

Looks fine, a few minor changes required though.

(Eventually, the toolchain abstraction work will also abstract the linker and binutils, so TOPT will be dealt with)

-m32
-MMD
-MP
${ARCH_FLAG}
-include ${ZEPHYR_BASE}/arch/posix/include/posix_cheats.h
)

if(NOT CMAKE_C_COMPILER_ID STREQUAL "Clang") # clang errors on -fno-freestanding
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are many toolchains that are not clang.
I'd suggest the inverse: If GNU, rather than if not Clang.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mped-oticon

Makes sense. Fixed, have a look.

zephyr_compile_definitions(
_FORTIFY_SOURCE=2
)
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is for gcc, but until now clang has been reusing gcc stuff. But now there is a difference, so now clang does need its own target_security_fortify.cmake.


# Load toolchain_cc-family macros
# Significant overlap with freestanding gcc compiler so reuse it
include(${ZEPHYR_BASE}/cmake/compiler/gcc/target_security_fortify.cmake)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be ${ZEPHYR_BASE}/cmake/compiler/${COMPILER}/target_security_fortify.cmake

Copy link
Collaborator Author

@ozhuraki ozhuraki Mar 6, 2019

Choose a reason for hiding this comment

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

@mped-oticon

I just checked, clang doesn't understand fortify at all. Fixed, have a look.

@ozhuraki ozhuraki force-pushed the host-clang branch 2 times, most recently from d0f9d2c to 2b3763b Compare March 6, 2019 09:47
@ozhuraki
Copy link
Collaborator Author

ozhuraki commented Mar 6, 2019

@SebastianBoe

Would it be possible/desirable to have clang and llvm support host toolchains instead of introducing host-clang and host-llvm?

  1. It would be nice to simplify this a bit in all senses (including the elimination of host-llvm and host-clang folders), but I am not sure, I understand you here:
  • Merge cmake/toolchain/llvm and cmake/toolchain/host-llvm, cmake/compiler/clang and cmake/compiler/host-clang?
  • Drop cmake/toolchain/host-llvm and cmake/compiler/host-clang, if feasible?
  • -DCMAKE_C_COMPILER=clang while configuring the app?
  • Something else?
  1. What cmake/toolchain/llvm and cmake/compiler/clang are currently used for, for cross targets?

@ozhuraki
Copy link
Collaborator Author

ozhuraki commented Mar 6, 2019

@nashif @SebastianBoe @andrewboie @mped-oticon

And a wish for the native_posix is to make it optionally possible to relax –m32, so any host library can be linked into as is.

@SebastianBoe
Copy link
Collaborator

Merge cmake/toolchain/llvm and cmake/toolchain/host-llvm, cmake/compiler/clang and cmake/compiler/host-clang?

I'm suggesting to merge them, or in other words, have this PR patch the original build scripts instead of creating new ones.

@aescolar
Copy link
Member

aescolar commented Mar 6, 2019

And a wish for the native_posix is to make it optionally possible to relax –m32

Allowing people to override it would be ok (if they know what they are doing, and are willing to suffer the pain).
But note that the assumption that both pointers and ints are 32 bits is pervasive, and they are cast blindly back and forth in too many places. So the default should be that we compile for 32 bits.

@@ -30,7 +30,7 @@ set(CMAKE_SYSTEM_VERSION ${PROJECT_VERSION})
# We are not building dynamically loadable libraries
set(BUILD_SHARED_LIBS OFF)

if(NOT (COMPILER STREQUAL "host-gcc"))
if(NOT ((COMPILER STREQUAL "host-gcc") OR (COMPILER STREQUAL "host-clang")))
Copy link
Member

Choose a reason for hiding this comment

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

how is host-clang different from the current clang compiler definition we have?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nashif

how is host-clang different from the current clang compiler definition we have?

Not too much. After the discussion with @SebastianBoe, I am in progress of merging this with the existing cmake/toolchain/llvm and cmake/compiler/clang.

However I am missing something subtle there and can’t get it to work that way yet.

The original need is to use libFuzzer to fuzz a few states in the experimental TCP compiled as a native_posix application.

@nashif nashif added the Maintainer At least 2 days before merge, maintainer review required label Mar 6, 2019
@ozhuraki
Copy link
Collaborator Author

ozhuraki commented Mar 6, 2019

@SebastianBoe, @nashif

Merged with the existing cmake/toolchain/llvm and cmake/compiler/clang, take a look.

@ozhuraki
Copy link
Collaborator Author

ozhuraki commented Mar 6, 2019

@SebastianBoe, @nashif

Sorry, I pushed a wrong stuff, ignore please.

@ozhuraki
Copy link
Collaborator Author

ozhuraki commented Mar 6, 2019

@SebastianBoe, @nashif

This is the correct one, please take a look.

CMakeLists.txt Outdated
toolchain_cc_security_fortify()
if(COMMAND toolchain_cc_security_fortify)
toolchain_cc_security_fortify()
endif()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe that by-design toolchains that don't support this should declare NO-OP functions.

Correct me if I'm wrong @mped-oticon

@@ -1 +1,5 @@
find_program(CMAKE_C_COMPILER clang PATH ${TOOLCHAIN_HOME} NO_DEFAULT_PATH)
if(NOT "${ARCH}" STREQUAL "posix")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the NOT

@ozhuraki ozhuraki force-pushed the host-clang branch 2 times, most recently from c9142b2 to 0067801 Compare April 20, 2019 12:38
@ozhuraki
Copy link
Collaborator Author

@nashif

also, please add llvm to supported toolchain

Added.

I am getting this on fedora 29

Resolved, please take a look.

@nashif
Copy link
Member

nashif commented Apr 20, 2019

@ozhuraki looks better now, but if you rebase on top of latest master...

clang-7: error: unknown argument: '-fmacro-prefix-map=/home/nashif/Work/zephyrproject/zephyr=.'

@ozhuraki
Copy link
Collaborator Author

@nashif

but if you rebase on top of latest master...

OK, I will take a look.

ozhuraki added 14 commits April 21, 2019 12:28
When linking, clang doesn't pick -T for some reason and complains,
while -Wl,-T works for both, gcc and clang.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
Mention that host installed clang can be used for native_posix.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
Allow host installed clang to be used for native_posix when
ZEPHYR_TOOLCHAIN_VARIANT=llvm.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
Use it's own variable HOST_TOOLS_HOME for host tools and don't
unconditionally set TOOLCHAIN_HOME, preventing the detection of
llvm/clang host toolchain.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
Host-tools don't unconditionally set TOOLCHAIN_HOME anymore,
but in case Zephyr's SDK toolchain is used, set TOOLCHAIN_HOME.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
The logic is practically intact and is the following:

1. Use any host installed llvm/clang in the path in case
   ZEPHYR_TOOLCHAIN_VARIANT=llvm is requested alone.
2. This can be further restricted with TOOLCHAIN_HOME.
3. And can be further overridden with CLANG_ROOT_DIR,
   like previously.

So, only the unconditional restriction to /usr is lifted.

Together with fixing the unconditional set of TOOLCHAIN_HOME
by host tools for non-toolchain needs, this makes the logic
more flexible.

Now, after the logic is controllable by TOOLCHAIN_HOME, 3)
might be an extra, but is left intact for backward compatibility.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
In case TOOLCHAIN_HOME isn't explicitly reuqested,
(or indirectly forced with CLANG_ROOT_DIR), detect
any host installed clang in the path.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
clang doesn't understand fortify at all, provide no op macro,
in order to handle the request to fortify in a generic way.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
native_posix should build with standard includes.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
Fix comment, llvm -> clang.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
clang has problems compiling the native_posix with -fno-freestanding.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
Add llvm to supported toolchains.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
libgcc isn't used by native_posix.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
Invalidate toolchain capability cache on toolchain configuration.

Signed-off-by: Oleg Zhurakivskyy <[email protected]>
@ozhuraki
Copy link
Collaborator Author

@nashif

OK, reproduced. Looks like toolchain capability cache is in the way on Fedora. On Ubuntu, I don’t see such problem.

There’s probably less intrusive way to overpass this.

@nashif nashif merged commit cbe74d4 into zephyrproject-rtos:master Apr 30, 2019
@marc-hb
Copy link
Collaborator

marc-hb commented Apr 30, 2019

clang-7: error: unknown argument: '-fmacro-prefix-map=/home/nashif/Work/zephyrproject/zephyr=.'

@benpicco also hit this weird cache issue with just gcc, no clang involved. See his detailed and useful report in issue #15551

PS: I just discovered this PR in the git log. Considering my deterministic builds work in progress (see links in PR #14593) please copy me on major cmake and build PRs like this one, thanks!

@ozhuraki ozhuraki deleted the host-clang branch May 3, 2019 07:25
@Akirakato1 Akirakato1 mentioned this pull request Jan 29, 2020
@SebastianBoe
Copy link
Collaborator

Hi, @ozhuraki , this patch:

cbe74d4

slipped past review.

Can you elaborate more on the rationale, or steps-to-reproduce the issue faced than this:

OK, reproduced. Looks like toolchain capability cache is in the way on Fedora. On Ubuntu, I don’t see such problem.

@ozhuraki
Copy link
Collaborator Author

@SebastianBoe

steps-to-reproduce the issue

Changing in between gcc and clang for native_posix on Fedora was reliably triggering this.

On Ubuntu it wasn’t reproducible.

Removing the ${USER_CACHE_DIR} was fixing the issue. So, possibly something is wrong/incompatible with the toolchain capability database in such cases, hence this patch.

@marc-hb
Copy link
Collaborator

marc-hb commented Feb 11, 2020

"possibly" something is wrong

There should at the very least be a comment in the source saying this was a blind shot and at what.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: Toolchains Toolchains Maintainer At least 2 days before merge, maintainer review required
Projects
None yet
Development

Successfully merging this pull request may close these issues.