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

Use LLVM Tools #1031

Merged
merged 5 commits into from Jul 6, 2018

Conversation

Projects
None yet
4 participants
@bradjc
Copy link
Contributor

bradjc commented Jun 26, 2018

Pull Request Overview

At long last, this pull request removes the need for arm-none-eabi-gcc and instead uses the llvm tools (size, objcopy, and objdump).

This requires the nightly upgrade and the switch to lld, so it really needs to be viewed with just individual commits.

Testing Strategy

This pull request was tested by programming the kernel on hail.

TODO or Help Wanted

n/a

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make formatall.

@niklasad1 niklasad1 referenced this pull request Jun 26, 2018

Closed

Tracking: Minimize dependencies to build kernel #866

4 of 5 tasks complete

@niklasad1 niklasad1 added the blocked label Jun 26, 2018

@niklasad1

This comment has been minimized.

Copy link
Member

niklasad1 commented Jun 26, 2018

This PR depends on #997 right?!

@bradjc bradjc force-pushed the llvm-binutils3 branch from 2daf998 to 199db68 Jun 27, 2018

@bradjc bradjc removed the blocked label Jun 27, 2018

# lld the actual page size so it doesn't have to be conservative.
RUSTFLAGS_FOR_CARGO_LINKING := "-C link-arg=-Tlayout.ld -C linker=lld \
-Z linker-flavor=ld.lld -C relocation-model=dynamic-no-pic \
-C link-arg=-zmax-page-size=512"

This comment has been minimized.

@ppannuto

ppannuto Jun 27, 2018

Member

Is this page size the flash page size? Isn't this chip-specific? (Do all our chips happen to have 512 byte pages right now?)

This comment has been minimized.

@bradjc

bradjc Jun 27, 2018

Contributor

I think so, but exists so that something can configure particular settings on particular flash pages, which we don't do. We need it set to 512 (or smaller) to match this: https://github.com/tock/tock/pull/1031/files#diff-d5b6e65f5bfb42e94ee9772ef4ac3b07L143 so that the relocate section is placed after text without a gap.

This comment has been minimized.

@ppannuto

ppannuto Jun 27, 2018

Member

Ah, makes sense. I think we'll probably have to come up with something for configurable page sizes some day, but that doesn't feel like a today problem.

I wish the github UI had some way of marking a comment as "resolved" so it'd collapse automatically.

* Tock places the kernel stack at the bottom of SRAM so that the
* kernel will trigger memory fault if it exceeds its stack depth,
* rather than silently overwriting valuable data.
*/

This comment has been minimized.

@ppannuto

ppannuto Jun 27, 2018

Member

This comment is no longer true... which is sad :/

Debugging stack overflows is much harder when the stack overwrites the relocation section :/

This comment has been minimized.

@bradjc

bradjc Jun 27, 2018

Contributor

Maybe it just needs someone else to try!

This comment has been minimized.

@bradjc

bradjc Jun 27, 2018

Contributor

One option I know that we do have is to leave a STACK sized gap (which would just be wasted space) between text and the relocate sections and change the kernel to look for relocate symbols later in flash. Then the stack could stay where it is.

@bradjc bradjc force-pushed the llvm-binutils3 branch from 03db59d to 79af4ba Jun 28, 2018

@alevy alevy added the P-Significant label Jul 4, 2018

@ppannuto
Copy link
Member

ppannuto left a comment

I'd like to resolve the stack moving problem -- either via a temporary situation with stack padding while we work to improve LLD tools or finding a solution that doesn't require that -- before merging this.

I think allowing stack overflows to overwrite relocations is a pretty major regression.

@alevy alevy dismissed their stale review via 106c40e Jul 5, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 5, 2018

Moving the stack section declaration to the very beginning of the linker script seems to do the trick. Because it has no output in the binary, it doesn't affect the offsets of sections or symbols written to ROM, but it ensures the stack is the very first thing in RAM.

Tested briefly by building for Hail and running, as well as looking at the section offsets in the ELF and binary file, but worth verifying with a few more apps.

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 5, 2018

Tested with a few other apps including our MPU tests (which sadly don't test the stack [correctly, it's really hard to write a program that uses exactly maximum stack]). Seems to be working to me.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 5, 2018

Moving the stack section declaration to the very beginning of the linker script seems to do the trick. Because it has no output in the binary, it doesn't affect the offsets of sections or symbols written to ROM, but it ensures the stack is the very first thing in RAM.

Writing/editing linker files is not a one person job :) Excellent find.

@ppannuto ppannuto added the last-call label Jul 5, 2018

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 5, 2018

I think this has been floating out here for a while and is in a pretty good place. Let's start a 24h clock to merging for this fellow.

bradjc and others added some commits Jun 21, 2018

use llvm-tools
Rust nightly now ships with a `llvm-tools` component. This means we can
use the llvm version of size and objcopy rather than requiring
arm-none-eabi-gcc.
need to put stack after relocate :(
When objcopy creates the binary it leaves space for the stack in the
binary. Moving the stack after the relocate section puts the relocate
section immediately after the text sections in the binary, which is
where it needs to be.

I'm not sure if there is a way to change the elf or pass flags to
objcopy that would make this not the case.
Move stack section to beginning of linker script
Ensures that the stack is the very first thing in RAM, meaning a stack
overflow will cause a fault as opposed to writing over other RAM.

@bradjc bradjc dismissed stale reviews from alevy and ppannuto via 6b61c88 Jul 6, 2018

@bradjc bradjc force-pushed the llvm-binutils3 branch from 106c40e to 6b61c88 Jul 6, 2018

@ppannuto
Copy link
Member

ppannuto left a comment

Re-approved. New changes are the documentation updates.

@bradjc bradjc merged commit 168bffc into master Jul 6, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@bradjc bradjc deleted the llvm-binutils3 branch Jul 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment