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

build system: fix compilation with LLVM #18845

Merged
merged 2 commits into from
Nov 7, 2022
Merged

Conversation

maribu
Copy link
Member

@maribu maribu commented Nov 4, 2022

Contribution description

  • makefiles/cflags.inc.mk: add -z noexecstack to link flags
    • GCC on some platforms does need an executable stack to generate trampoline code, but use of nested functions is not allowed in RIOT's code base anyway.
  • makefiles/libc: fix wchar_t size warnings on LLVM
    • The test for the requirement of disabling wchar_t size warnings assumed that $(CC) is used for linking, instead of $(LINK). With GCC $(LINK) and $(CC) happen to be (in most cases) identical, but with LLVM they are not. This results in issues with compiling with LLVM.

Testing procedure

No change in binaries and everything should still compile.

Issues/PRs references

Inspired by #18844

@github-actions github-actions bot added the Area: build system Area: Build system label Nov 4, 2022
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 4, 2022
@riot-ci
Copy link

riot-ci commented Nov 4, 2022

Murdock results

✔️ PASSED

0077e9a makefiles/libc: fix wchar_t size warnings on LLVM

Success Failures Total Runtime
2000 0 2000 07m:17s

Artifacts

This only reflects a subset of all builds from https://ci-prod.riot-os.org. Please refer to https://ci.riot-os.org for a complete build for now.

@benpicco
Copy link
Contributor

benpicco commented Nov 4, 2022

Our msp430 toolchain is too old to know about -z noexecstack 😞

@chrysn
Copy link
Member

chrysn commented Nov 4, 2022

Thanks, this also Closes: #18522.

On the topic of not having an executable stack, my assumptions here are that:

  1. We may at some point use any present MMUs to enforce that, but doing that is out of scope for this PR.
  2. If some code does need to use an executable stack (say, because a third party library is trampolining around, or something JITish is added), then they can still propose a global switch (eg. a stack_is_execuable pseudomodule) that would turn the explicit noexecstack into an explicit execstack and disables any of 1. when needed, but implementing that is also out of scope for here.

Do they align with your intentions for this PR?

@maribu
Copy link
Member Author

maribu commented Nov 4, 2022

Our msp430 toolchain is too old to know about -z noexecstack 😞

That's a pity. The msp430 toolchain is relatively straight forward to build...

makefiles/cflags.inc.mk Outdated Show resolved Hide resolved
@maribu
Copy link
Member Author

maribu commented Nov 7, 2022

  1. If some code does need to use an executable stack (say, because a third party library is trampolining around, or something JITish is added), then they can still propose a global switch (eg. a stack_is_execuable pseudomodule) that would turn the explicit noexecstack into an explicit execstack and disables any of 1. when needed, but implementing that is also out of scope for here.

Adding a -z execstack add the end of the link facts should take preference over the -z noexecstack, so it should be possible to counter this if ever needed.

However, I don't think that this is too likely needed. E.g. the JIT support of WASM increases the resource requirements so significantly that it becomes challenging to fit the constrained memory of typical RIOT hardware. But it does remain a theoretical option, nonetheless.

makefiles/cflags.inc.mk Outdated Show resolved Hide resolved
GCC on some platforms does need an executable stack to generate
trampoline code, but use of nested functions is not allowed in
RIOT's code base anyway.
The test for the requirement of disabling wchar_t size warnings assumed
that $(CC) is used for linking, instead of $(LINK). With GCC $(LINK)
and $(CC) happen to be (in most cases) identical, but with LLVM they
are not. This results in issues with compiling with LLVM.
@benpicco benpicco merged commit 8a811e8 into RIOT-OS:master Nov 7, 2022
@maribu maribu deleted the llvm-compat branch November 7, 2022 18:54
@maribu
Copy link
Member Author

maribu commented Nov 7, 2022

Thx :)

@maribu
Copy link
Member Author

maribu commented Nov 17, 2022

This apparently needs a backport. Building rust fails in the release branch for me, but works in master. Cherry-picking this fixes the issue.

Also, we really do want LLVM support (even beside rust) in the release, don't we?

@maribu
Copy link
Member Author

maribu commented Nov 17, 2022

Backport provided in #18928

@kaspar030 kaspar030 added this to the Release 2023.01 milestone Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants