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

Fix native_posix build on Debian #38903

Merged
merged 1 commit into from Oct 5, 2021
Merged

Conversation

yperess
Copy link
Collaborator

@yperess yperess commented Sep 28, 2021

Debian (by default) enables PIE/PIC and the flag to override it doesn't
use the 'f' prefix. This is fixed by also adding -no-pie and -no-pic
to the zephyr_cc_options.

Issue #35244

Signed-off-by: Yuval Peress peress@google.com

@yperess
Copy link
Collaborator Author

yperess commented Sep 28, 2021

@yashi This fixes the issue on my device. Could you give it a try?

@yperess
Copy link
Collaborator Author

yperess commented Sep 28, 2021

Set as draft, it seems to break other builds. I'll keep playing around with these flags.

@tejlmand
Copy link
Collaborator

it's most likely related to this binutils commit:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=fa6ecf940581b4be26165351bb0473427d31c7d8

but note, this is only a linker warning, but because all warnings are treated as errors per default when using twister the end result is an error.

@yperess yperess marked this pull request as ready for review September 30, 2021 19:10
@yperess
Copy link
Collaborator Author

yperess commented Sep 30, 2021

I also considered putting this under cmake/compiler/**/*.cmake but it seemed like it would be a lot of repeated commands. I'm open to trying it.

@yashi
Copy link
Collaborator

yashi commented Oct 1, 2021

@yashi This fixes the issue on my device. Could you give it a try?

I've missed this somehow. I'll find time to test it today. meanwhile,

The GCC compiler option for no-pie/no-pic doesn't include the 'f' prefix.

Is this really true? I see -fno-pie, -fno-pic, and -no-pie in the GCC repo. but no mention of -no-pic.

GCC info has -no-pie in the section "3.15 Options for Linking", but not -no-pic. (emphasis mine)

@yperess
Copy link
Collaborator Author

yperess commented Oct 1, 2021

@yashi This fixes the issue on my device. Could you give it a try?

I've missed this somehow. I'll find time to test it today. meanwhile,

The GCC compiler option for no-pie/no-pic doesn't include the 'f' prefix.

Is this really true? I see -fno-pie, -fno-pic, and -no-pie in the GCC repo. but no mention of -no-pic.

GCC info has -no-pie in the section "3.15 Options for Linking", but not -no-pic. (emphasis mine)

You're right about -no-pic it looks like it's an architecture specific and should be prefixed with m, so -mno-pic. I'm seeing a reference to -fno-pic but only in the documentation under -miamcu. I'm not seeing anything about -fno-pie.

Easiest reference I've been looking at is at https://man7.org/linux/man-pages/man1/gcc.1.html, do you have a better one?

@yashi
Copy link
Collaborator

yashi commented Oct 1, 2021

How about something like this:

diff --git a/cmake/linker/ld/target_baremetal.cmake b/cmake/linker/ld/target_baremetal.cmake
index 36ec3c14d8..76965ed81b 100644
--- a/cmake/linker/ld/target_baremetal.cmake
+++ b/cmake/linker/ld/target_baremetal.cmake
@@ -8,7 +8,6 @@ macro(toolchain_ld_baremetal)
   zephyr_ld_options(
     -nostdlib
     -static
-    -no-pie
     ${LINKERFLAGPREFIX},-X
     ${LINKERFLAGPREFIX},-N
   )
diff --git a/cmake/linker/ld/target_base.cmake b/cmake/linker/ld/target_base.cmake
index 0eee30ea85..1fec5ca96f 100644
--- a/cmake/linker/ld/target_base.cmake
+++ b/cmake/linker/ld/target_base.cmake
@@ -11,6 +11,7 @@ macro(toolchain_ld_base)
   # TOOLCHAIN_LD_FLAGS comes from compiler/gcc/target.cmake
   # LINKERFLAGPREFIX comes from linker/ld/target.cmake
   zephyr_ld_options(
+    -no-pie
     ${TOOLCHAIN_LD_FLAGS}
   )
 

We unconditionally set -fno-pie for "code generation" in the root CMakeLists.txt, why not set -no-pie for "linking"? zephyr_ld_options() checks to see the option is supported by the selected compiler. Unconditionally setting no-pie states that we want the final binary to be non position independent executable in all cases.

@yperess
Copy link
Collaborator Author

yperess commented Oct 1, 2021

How about something like this:

diff --git a/cmake/linker/ld/target_baremetal.cmake b/cmake/linker/ld/target_baremetal.cmake
index 36ec3c14d8..76965ed81b 100644
--- a/cmake/linker/ld/target_baremetal.cmake
+++ b/cmake/linker/ld/target_baremetal.cmake
@@ -8,7 +8,6 @@ macro(toolchain_ld_baremetal)
   zephyr_ld_options(
     -nostdlib
     -static
-    -no-pie
     ${LINKERFLAGPREFIX},-X
     ${LINKERFLAGPREFIX},-N
   )
diff --git a/cmake/linker/ld/target_base.cmake b/cmake/linker/ld/target_base.cmake
index 0eee30ea85..1fec5ca96f 100644
--- a/cmake/linker/ld/target_base.cmake
+++ b/cmake/linker/ld/target_base.cmake
@@ -11,6 +11,7 @@ macro(toolchain_ld_base)
   # TOOLCHAIN_LD_FLAGS comes from compiler/gcc/target.cmake
   # LINKERFLAGPREFIX comes from linker/ld/target.cmake
   zephyr_ld_options(
+    -no-pie
     ${TOOLCHAIN_LD_FLAGS}
   )
 

We unconditionally set -fno-pie for "code generation" in the root CMakeLists.txt, why not set -no-pie for "linking"? zephyr_ld_options() checks to see the option is supported by the selected compiler. Unconditionally setting no-pie states that we want the final binary to be non position independent executable in all cases.

That works on my machine, update the PR and we'll see what the CI thinks :)

@yashi yashi self-requested a review October 2, 2021 10:05
CMakeLists.txt Outdated
@@ -302,7 +302,6 @@ zephyr_compile_options($<$<COMPILE_LANGUAGE:CXX>:$<TARGET_PROPERTY:compiler-cpp,
include(cmake/extra_flags.cmake)

zephyr_cc_option(-fno-asynchronous-unwind-tables)
zephyr_cc_option(-fno-pie)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you want to remove this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't it now added in ld/target_base.cmake anyway? Did I misunderstand how that .cmake file gets included?

Copy link
Collaborator

@yashi yashi left a comment

Choose a reason for hiding this comment

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

Update -fno-pie to only be added when relevant. This fixes native_posix builds on Debian hosts.

This doesn't describe what the error is nor why we add this change. A few points we should make in the commit log are:

  • Debian and other Linux distributions have ld with --warn-textrel as default.
  • The option generates a warning when linking on x86.
  • The warning make scripts/twister fail because we treat all warnings as error.
  • The warning is issued when no PIE objects are linked against PIE library.
  • PIE (Position Independent Executable) or not can be seen with the following command.
  • When you want to generate non PIE, link with -no-pie.

Non PIE

$ readelf -l build/zephyr/zephyr.elf | grep 'Elf file type is'
Elf file type is EXEC (Executable file)

PIE

$ readelf -l /lib/x86_64-linux-gnu/libc.so.6 | grep 'Elf file type is'
Elf file type is DYN (Shared object file)

@yperess
Copy link
Collaborator Author

yperess commented Oct 2, 2021

Update -fno-pie to only be added when relevant. This fixes native_posix builds on Debian hosts.

This doesn't describe what the error is nor why we add this change. A few points we should make in the commit log are:

  • Debian and other Linux distributions have ld with --warn-textrel as default.
  • The option generates a warning when linking on x86.
  • The warning make scripts/twister fail because we treat all warnings as error.
  • The warning is issued when no PIE objects are linked against PIE library.
  • PIE (Position Independent Executable) or not can be seen with the following command.
  • When you want to generate non PIE, link with -no-pie.

Non PIE

$ readelf -l build/zephyr/zephyr.elf | grep 'Elf file type is'
Elf file type is EXEC (Executable file)

PIE

$ readelf -l /lib/x86_64-linux-gnu/libc.so.6 | grep 'Elf file type is'
Elf file type is DYN (Shared object file)

Done, I added this content. Sorry, I assumed all the rest of the info could be found by following the issue link.

Update -fno-pie to only be added when relevant. This fixes native_posix
builds on Debian hosts.

* Debian and other Linux distributions have ld with --warn-textrel as
default.
* The option generates a warning when linking on x86.
* The warning make scripts/twister fail because we treat all warnings
as error.
* The warning is issued when no PIE objects are linked against PIE
library.
* PIE (Position Independent Executable) or not can be seen with the
following command.
* When you want to generate non PIE, link with `-no-pie.`

Non PIE

  $ readelf -l build/zephyr/zephyr.elf | grep 'Elf file type is'
  Elf file type is EXEC (Executable file)

PIE

  $ readelf -l /lib/x86_64-linux-gnu/libc.so.6 | \
  grep 'Elf file type is'
  Elf file type is DYN (Shared object file)

Issue zephyrproject-rtos#35244

Signed-off-by: Yuval Peress <peress@google.com>
Copy link
Collaborator

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

lgtm

@nashif nashif merged commit a81f8af into zephyrproject-rtos:main Oct 5, 2021
chrta added a commit to lemonbeat/zephyr that referenced this pull request Nov 2, 2022
See also
zephyrproject-rtos#38903

This is required when building tests for native_posix on ubuntu 22.04 using
clang-14 from the normal deb repository.

Signed-off-by: Christian Taedcke <christian.taedcke@lemonbeat.com>
rettichschnidi pushed a commit to husqvarnagroup/zephyr that referenced this pull request Nov 14, 2022
See also
zephyrproject-rtos#38903

This is required when building tests for native_posix on ubuntu 22.04 using
clang-14 from the normal deb repository.

Signed-off-by: Christian Taedcke <christian.taedcke@lemonbeat.com>
(cherry picked from commit 017fdc0a8769ab15172ad11e951ed9be1184d631)
stephanosio pushed a commit that referenced this pull request Nov 22, 2022
See also
#38903

This is required when building tests for native_posix on ubuntu 22.04 using
clang-14 from the normal deb repository.

Signed-off-by: Christian Taedcke <christian.taedcke@lemonbeat.com>
coreboot-org-bot pushed a commit to coreboot/zephyr-cros that referenced this pull request Nov 25, 2022
See also
zephyrproject-rtos/zephyr#38903

This is required when building tests for native_posix on ubuntu 22.04 using
clang-14 from the normal deb repository.

(cherry picked from commit 1fde62e)

Original-Signed-off-by: Christian Taedcke <christian.taedcke@lemonbeat.com>
GitOrigin-RevId: 1fde62e
Change-Id: I76cfb2bbd71df1ffb137b681b648320081f9f046
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/4047582
Commit-Queue: Fabio Baltieri <fabiobaltieri@google.com>
Reviewed-by: Yuval Peress <peress@google.com>
Tested-by: CopyBot Service Account <copybot.service@gmail.com>
Reviewed-by: Fabio Baltieri <fabiobaltieri@google.com>
Reviewed-by: Keith Short <keithshort@chromium.org>
Tested-by: Yuval Peress <peress@google.com>
Tested-by: Fabio Baltieri <fabiobaltieri@google.com>
Tested-by: Keith Short <keithshort@chromium.org>
jtbaumann pushed a commit to tmobile/DevEdge-IoTDevKit-ZephyrRTOS that referenced this pull request Dec 12, 2022
See also
zephyrproject-rtos/zephyr#38903

This is required when building tests for native_posix on ubuntu 22.04 using
clang-14 from the normal deb repository.

Signed-off-by: Christian Taedcke <christian.taedcke@lemonbeat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants