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 LLD linker #993

Merged
merged 3 commits into from Jun 26, 2018

Conversation

Projects
None yet
3 participants
@bradjc
Copy link
Contributor

bradjc commented Jun 13, 2018

Pull Request Overview

This pull request changes the build system to use LLVM LLD rather than GNU LD as the linker.

In theory this is a very minor change. In practice, two things are different:

  1. The MEMORY command in the linker file does not work with variables. Therefore, each platform will have to define its own memory section. I have done this for hail, and can apply it more broadly if everyone is cool with this PR.

  2. Simply doing . = . + STACK_SIZE; is no longer a valid way to set up the .stack section so that it is the correct size. The resulting .elf has a .stack section which is the correct length, but the next section (.relocate) is placed on top of it (i.e. it is at the same address). I have found two workarounds for this, the first is to use 512 QUAD(0x00) statements in the linker file to fill the space. This kinda works, in that it makes the .stack section the correct size, but for some reason the resulting .bin file becomes 512MB so the kernel doesn't actually work. The other solution is the one in this PR: make a dummy array in rust and then assign it to the section.

The faulty .elf:

Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            00000000 000000 000000 00      0   0  0
  [ 1] .text             PROGBITS        00010000 001000 015230 00 AXM  0   0 32
  [ 2] .ARM.exidx        ARM_EXIDX       00025230 016230 000010 00  AL  1   0  4
  [ 3] .storage          NOBITS          0002523f 016240 0001c1 00   A  0   0  1
  [ 4] .apps             NOBITS          00030000 017000 000000 00   A  0   0  1
  [ 5] .stack            NOBITS          20000000 017000 001000 00   A  0   0  1
  [ 6] .relocate         PROGBITS        20000000 017000 000e2c 00  WA  0   0  8
  [ 7] .sram             NOBITS          20000e30 017e2c 00f1d0 00  WA  0   0 16

The workaround is a little unfortunate, but maybe either I am missing something or LLD 7.0 will fix this.

Testing Strategy

This pull request was tested by running hail on hail.

TODO or Help Wanted

This pull request still needs to be updated for all other boards.

Also, I think this is worth it to get to fewer dependencies and better exist in the rust ecosystem, but others may have different thoughts.

Documentation Updated

  • Kernel: Updated the relevant files in /docs, or no updates are required.
  • Userland: Added/updated the application README, if needed.

Formatting

  • Ran make formatall.
@ppannuto
Copy link
Member

ppannuto left a comment

A nice notion to lessen toolchains required, but I don't think LLD is ready for primetime yet :/

@@ -226,7 +216,7 @@ SECTIONS

. = ALIGN(4);
_erelocate = .;
} > ram AT > rom
} > ram

This comment has been minimized.

@ppannuto

ppannuto Jun 13, 2018

Member

This is a regression too, and a pretty serious one. This syntax ensures that this section is charged to both the ram and rom sections. The AT syntax simply places it there without accounting for size. This silent failure was the root cause of a pretty awful to track down signpost bug.

This comment has been minimized.

@bradjc

bradjc Jun 13, 2018

Contributor

AT > rom doesn't seem to work with lld. Instead, I added an assert to the bottom of the linker script. Take a look and see if that would also catch the error.

This comment has been minimized.

@ppannuto

ppannuto Jun 14, 2018

Member

Haven't tested, but shouldn't that be < LENGTH(rom) not > LENGTH(rom)?

This comment has been minimized.

@bradjc

bradjc Jun 14, 2018

Contributor

It only compiles this way.

/// stack.
#[no_mangle]
#[link_section = ".stack_buffer"]
pub static mut STACK_MEMORY: [u8; 0x1000] = [0; 0x1000];

This comment has been minimized.

@ppannuto

ppannuto Jun 13, 2018

Member

I'm generally adverse to having the same information duplicated in two places. Is the STACK_SIZE variable in the linker script still used with this approach or could it possible be dropped?

This comment has been minimized.

@bradjc

bradjc Jun 13, 2018

Contributor

Removed.

{
rom (rx) : ORIGIN = 0x00010000, LENGTH = 0x00020000
prog (rx) : ORIGIN = 0x00030000, LENGTH = 0x00040000
ccfg (rx) : ORIGIN = 0, LENGTH = 0

This comment has been minimized.

@ppannuto

ppannuto Jun 13, 2018

Member

I'm a little wary of the copy/paste that this introduces here. i.e. the ccfg section doesn't really make sense to be in the hail memory map at all? Granted it was there in the general case, but that's hidden away. Now that it's in the "hail-specific" file, it's odd to see it.

This comment has been minimized.

@bradjc

bradjc Jun 13, 2018

Contributor

Yeah that can be removed from boards that do not use it.

This comment has been minimized.

@ppannuto

ppannuto Jun 14, 2018

Member

If you remove the ccfg definition here, does this part of the linker file cause problems?

    .ccfg : {
        KEEP(*(.ccfg))
    } > ccfg

Or does it just silently ignore undefined, empty things?

This comment has been minimized.

@bradjc

bradjc Jun 14, 2018

Contributor

Doesn't seem to mind.

@@ -7,3 +8,11 @@ RAM_ORIGIN = 0x20000000;
RAM_LENGTH = 0x00020000;

MPU_MIN_ALIGN = 8K;

MEMORY

This comment has been minimized.

@ppannuto

ppannuto Jun 14, 2018

Member

I think if we do this we can kill off all the variables above too. Doesn't look like they're used anywhere else.

This comment has been minimized.

@bradjc

bradjc Jun 14, 2018

Contributor

Gone.

@alevy alevy added the P-Significant label Jun 16, 2018

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jun 19, 2018

While I appreciate @ppannuto's concerns, I find it suspicious we wouldn't be able to link the kernel with LLD, given that the much more complicated libtock-rs userland is linked with LLD---it's just a fairly straight forward linking process that's only somewhat complicated due to our build system helping avoid duplication across boards.

My question is how to best stress test this. One option is to merge, see if/how it starts breaking for people before the next release, and fix or revert before then. Another option, would be to somehow test rigorously before merging. I'm personally more sympathetic to merging and fixing, since I suspect this is probably pretty close.

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jun 19, 2018

@niklasad1 niklasad1 referenced this pull request Jun 19, 2018

Closed

Tracking: Minimize dependencies to build kernel #866

4 of 5 tasks complete

bradjc added some commits Jun 19, 2018

boards: use llvm lld
This switches the build system to use the LLVM linker (lld) that ships
with rust rather than arm-none-eabi-gcc.
boards: update all boards to use new linker file
Variables do not work the same as in gnu ld, so we have to include the
`MEMORY` section directly in each board's linker file.

@bradjc bradjc force-pushed the llvm-lld branch from 8153fe7 to be73fa7 Jun 19, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jun 19, 2018

Testing is good. I just updated so that all boards now use this.

Also I was totally wrong about the ASSERT; I wasn't taking into account that the kernel on hail starts at 0x10000.

@alevy

alevy approved these changes Jun 26, 2018

Monday night has passd.

@bradjc bradjc merged commit 1a66eb0 into master Jun 26, 2018

2 checks passed

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

@bradjc bradjc deleted the llvm-lld branch Jun 26, 2018

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