Skip to content

Conversation

@stephanosio
Copy link
Member

@stephanosio stephanosio commented Sep 12, 2021

This commit updates the Zephyr build system such that it does not
include the zephyr_stdint.h, which tries to define Zephyr's own type
system, when compiling for the native POSIX architecture.

The native POSIX architecture compiles with the host toolchain and
headers, and there can be conflicts if we arbitrarily define our own
type system (e.g. mismatch between the types and the specifiers defined
in inttypes.h).

Note that this is not meant to be a permanent fix; instead, it is meant
to serve as a temporary workaround until we no longer need to define
our own type system.

For more details, refer to the issue #37718.

Signed-off-by: Stephanos Ioannidis [email protected]

Fixes #37718.

This commit updates the Zephyr build system such that it does not
include the `zephyr_stdint.h`, which tries to define Zephyr's own type
system, when compiling for the native POSIX architecture.

The native POSIX architecture compiles with the host toolchain and
headers, and there can be conflicts if we arbitrarily define our own
type system (e.g. mismatch between the types and the specifiers defined
in `inttypes.h`).

Note that this is not meant to be a permanent fix; instead, it is meant
to serve as a temporary workaround until we no longer need to define
our own type system.

For more details, refer to the issue zephyrproject-rtos#37718.

Signed-off-by: Stephanos Ioannidis <[email protected]>
@stephanosio stephanosio marked this pull request as ready for review September 12, 2021 12:00
@stephanosio stephanosio requested a review from galak September 12, 2021 12:00
@stephanosio stephanosio added area: native port Host native arch port (native_sim) area: Portability Standard compliant code, toolchain abstraction area: Toolchains Toolchains bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug labels Sep 12, 2021
@stephanosio stephanosio added this to the v2.7.0 milestone Sep 12, 2021
@cfriedt
Copy link
Member

cfriedt commented Sep 18, 2021

@nashif - also fairly trivial build-related issue. Can you ack?

@cfriedt
Copy link
Member

cfriedt commented Sep 20, 2021

@tejlmand - please have a review

# `zephyr_stdint.h` is not included for the POSIX (native) arch because it
# compiles with the host toolchain/headers and there can be conflicts if we
# arbitrarily redefine our own type system (see #37718).
zephyr_compile_options("SHELL: $<TARGET_PROPERTY:compiler,imacros> ${ZEPHYR_BASE}/include/toolchain/zephyr_stdint.h")
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the comment above be inside the if ?

# @Intent: Enforce standard integer type correspondance to match Zephyr usage.
# (must be after compiler specific flags)

As I see it, that comment belongs with the zephyr_compile_options(...) call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, that comment describes the following "code block" which includes the zephyr_compile_options inside if.

In this file, we seem to have multiple instances of the @Intent comment being placed similarly:

zephyr/CMakeLists.txt

Lines 109 to 112 in d7b5c3b

# @Intent: Set compiler flags to detect general stack overflows across all functions
if(CONFIG_STACK_CANARIES)
zephyr_compile_options($<TARGET_PROPERTY:compiler,security_canaries>)
endif()

zephyr/CMakeLists.txt

Lines 165 to 170 in d7b5c3b

# @Intent: Obtain compiler specific flags for compiling under different ISO standards of C++
if(CONFIG_CPLUSPLUS)
# From kconfig choice, pick a single dialect.
# Kconfig choice ensures only one of these CONFIG_STD_CPP* is set.
if(CONFIG_STD_CPP98)
set(STD_CPP_DIALECT_FLAGS $<TARGET_PROPERTY:compiler-cpp,dialect_cpp98>)

zephyr/CMakeLists.txt

Lines 282 to 286 in d7b5c3b

# @Intent: Add cmake -DW toolchain supported warnings, if any
if(W MATCHES "1")
zephyr_compile_options($<$<COMPILE_LANGUAGE:C>:$<TARGET_PROPERTY:compiler,warning_dw_1>>)
zephyr_compile_options($<$<COMPILE_LANGUAGE:CXX>:$<TARGET_PROPERTY:compiler-cpp,warning_dw_1>>)
endif()

This is really just a style issue, so I will relocate that comment in this PR if you prefer it that way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see your point.
But the other examples closely follows what they are testing, like:

  • general stack overflows comment follows if(CONFIG_STACK_CANARIES)
  • C++ comment follows the if(CONFIG_CPLUSPLUS)
  • -DW comment follows if(W MATCHES "1")

whereas this comment talks about enforcing standard integer types, so the relation from that comment to ARCH_POSIX is less obvious.

Maybe the best solution is to not move that comment, but instead update it, so that it is clear from the @Intent comment that it only applies for not native posix.

Copy link
Contributor

Choose a reason for hiding this comment

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

but my main reason for original comment was to be sure it was forgotten by mistake, and based on your answer it seems to be on purpose.

Copy link
Contributor

@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.

changes looks good, and @stephanosio already replied regarding my comment which ensures it was not an oversight.

@cfriedt cfriedt merged commit a1a6619 into zephyrproject-rtos:main Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Build System area: native port Host native arch port (native_sim) area: Portability Standard compliant code, toolchain abstraction area: Toolchains Toolchains bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incompatible (u)intptr_t type and PRIxPTR definitions

4 participants