Skip to content
This repository was archived by the owner on Nov 28, 2023. It is now read-only.

support linking with lld #10

Merged
merged 5 commits into from
Aug 18, 2018
Merged

support linking with lld #10

merged 5 commits into from
Aug 18, 2018

Conversation

danc86
Copy link
Contributor

@danc86 danc86 commented Aug 10, 2018

Rust now ships an embedded copy of lld, so if we can link crates with lld it will make the getting started experience way smoother (don't need to find a separate GNU ld).

@danc86
Copy link
Contributor Author

danc86 commented Aug 10, 2018

Hmm I should mark this as WIP.

This gets us most of the way, but I am still stuck on this bizarre error from lld when linking a simple program:

  = note: rust-lld: error: section '.rodata' will not fit in region 'FLASH': overflowed by 1589641216 bytes
          rust-lld: error: section '.data' will not fit in region 'FLASH': overflowed by 1589641216 bytes
          rust-lld: error: section '.data' will not fit in region 'FLASH': overflowed by 1589641216 bytes
          rust-lld: error: section '.data' will not fit in region 'FLASH': overflowed by 1589641216 bytes
          rust-lld: error: section '.got' will not fit in region 'FLASH': overflowed by 1589641216 bytes
          rust-lld: error: section '.got' will not fit in region 'FLASH': overflowed by 1589641216 bytes

I think the sections it's complaining about are actually 0 bytes. They are certainly not 1589641216 bytes. It seems like an arithmetic error somewhere. Maybe a bug in the riscv port?

1589641216 bytes is exactly 1516 MB which seems like an unusual number. It might be a clue as to what is going on.

@danc86 danc86 changed the title support linking with lld [WIP] support linking with lld Aug 10, 2018
@danc86
Copy link
Contributor Author

danc86 commented Aug 10, 2018

Another possibility is that I made a mistake in rebasing the lld patches. I had to rebase them onto an earlier commit of the 7.0 branch, to match the llvm commit that is in the rust tree. I could have messed something up in doing that. It would be worthwhile trying to use an lld built outside of Rust I guess.

The other thing I can do is go back over the link.x script in cortex-m-rt with a fine-toothed comb and see if I can spot anything else that I have done wrong here.

I will dig into it more over the weekend. If anyone else has any hints I am all ears :-)

@dvc94ch
Copy link
Member

dvc94ch commented Aug 10, 2018

You shouldn't have a .got in there. That's for a dynamic executable

@danc86
Copy link
Contributor Author

danc86 commented Aug 12, 2018

Yeah that is one of the things I don't understand yet, there shouldn't be a .got section, GNU ld rightly thinks there is not... I'm not sure why lld is complaining about it.

@dvc94ch dvc94ch mentioned this pull request Aug 12, 2018
@danc86
Copy link
Contributor Author

danc86 commented Aug 13, 2018

Oookay so I spent some time messing around inside gdb, I think it is a bug in lld... specifically the nonsensical error message occurs if you start a new section with a specific start address in a different region than the section before it. The bug is that it tries to expand the previous output section to meet the new address.

So one problematic bit is when we have:

...
  .rodata ALIGN(4) :
  {
    *(.rodata .rodata.*);
  } > FLASH

  PROVIDE(_sbss = ORIGIN(RAM));
  .bss _sbss :
  {
...

Even though .bss goes into the RAM region and not the FLASH region, lld was expanding the FLASH region from 0x20400000 to the start of RAM at 0x80000000, because we told it to start the section at the specific address of the _sbss symbol.

It seems like the easiest workaround is to avoid starting new sections from a fixed address, and let lld lay them out as it sees fit. In this case it will get the sizes right. So instead of the above, we would do:

.bss : {
...
}
_sbss = ADDR(.bss);

That is what cortex-m-rt has. The only downside is we lose the ability for the user to pass in their own separate linker script which overrides the start of the .text and .bss sections. That is what the PROVIDE() macro was allowing.

@danc86 danc86 requested a review from a team as a code owner August 13, 2018 13:01
@danc86
Copy link
Contributor Author

danc86 commented Aug 13, 2018

Okay, I think I got it. :-) The amended series now produces working binaries with lld and I don't think I have messed anything up. I have also learnt more than I ever wanted to know about linker scripts :-P

@danc86
Copy link
Contributor Author

danc86 commented Aug 13, 2018

Ah, sigh... I did mess it up. It no longer links with GNU ld.

          riscv64-unknown-elf-ld: address 0x100004004 of /home/dan/embedded-rust/riscv-rust-quickstart/target/riscv32imac-unknown-none-elf/debug/deps/riscv_rust_quickstart-e26cfc0aed2e7c57 section `.stack' is not within region `RAM'

That address does not make much sense to me, not sure where it is getting that from... Oh well. I will have another crack at it tomorrow.

@dvc94ch
Copy link
Member

dvc94ch commented Aug 13, 2018

Did you see my comment on removing the cfi stuff?

Does the bt command still work in gdb without this? That's the reason I added it in the first place. Could be some other bug was somewhere that's fixed now...

@danc86
Copy link
Contributor Author

danc86 commented Aug 13, 2018

Oh sorry I totally you missed that comment. I didn't realise the CFI made a difference to gdb. I'm not sure how that's possible, if the .eh_frame section is garbaged collected from the final binary by the linker? Unless those .cfi macros produce some other import debugging info besides just the .eh_frame? I will dig into it more to see how it works.

I can drop that commit and instead we can leave the .cfi_* directives in there and just /DISCARD/ the .eh_frame in the linker script. Or we could even just leave it in the binary, it seems harmless and maybe we will have working panic unwinding at some point.

@dvc94ch
Copy link
Member

dvc94ch commented Aug 14, 2018

.debug_frame or .eh-frame are used by the debugger when showing the stack trace. As long as the stack trace still works without it we can remove it...

@dvc94ch
Copy link
Member

dvc94ch commented Aug 14, 2018

Maybe it also fixes all the gdb crashes I was having... xD

danc86 added 3 commits August 15, 2018 13:43
This is needed for lld, otherwise it will complain about section flag
mismatch:

    ld.lld: error: incompatible section flags for .text
    >>> target/riscv32imac-unknown-none/debug/deps/libriscv_rt-7850ee1a6233fbe9.rlib(riscv_rt-7850ee1a6233fbe9.4tmuw4s4crjeqbm5.rcgu.o):(.trap): 0x4
    >>> output section .text: 0x6
This is no longer necessary now that we can configure the target to
exclude the .debug_gdb_scripts section.
See: rust-lang/rust#53139
This aligns the VMA (virtual adddress at runtime):

    .rodata ALIGN(4) :
    {
        ...

whereas this aligns the LMA (load address):

    .rodata : ALIGN(4)
    {
        ...

If we ensure the VMA is aligned the linker will keep the corresponding
LMA in sync (and it will be aligned too).

Previously, by forcing the LMA to be aligned but leaving the VMA
unspecified, the linker would split .text and .rodata into two
separate loads because their addresses fell out of sync.
@danc86
Copy link
Contributor Author

danc86 commented Aug 15, 2018

Hmm yeah I don't seem to get any usable backtraces at all, regardless whether it's built with GNU ld or lld, or with or without the .cfi_* directives, with .eh_frame discarded or included in the binary... so now I'm not sure what I'm doing wrong. 🙄

@danc86
Copy link
Contributor Author

danc86 commented Aug 15, 2018

I did mess it up. It no longer links with GNU ld.

riscv64-unknown-elf-ld: address 0x100004004 of /home/dan/embedded-rust/riscv-rust-quickstart/target/riscv32imac-unknown-none-elf/debug/deps/riscv_rust_quickstart-e26cfc0aed2e7c57 section `.stack' is not within region `RAM'

This came from changing (INFO) to (NOLOAD). There is a subtle difference: (INFO) not only marks the section as non-loadable, it also marks it as non-allocatable. Otherwise the program header will try to load into address 0x80000000 with size 0x80004004 (that itself is surely a bug, the real size of the stack is 0x3ffc not 0x80004000). Hence the complaint about address 0x100004004 being out of range.

It turns out lld supposedly gained support for the (INFO) attribute back in February: https://bugs.llvm.org/show_bug.cgi?id=36298
but it complains about a syntax error still:

rust-lld: error: /home/dan/embedded-rust/riscv-rust-quickstart/target/riscv32imac-unknown-none-elf/debug/build/riscv-rt-88029df47ab93817/out/link.x:49: NOLOAD expected, but got INFO
          >>>   .heap _edata (INFO) :
          >>>                 ^

So I guess that is another lld bug...

@danc86
Copy link
Contributor Author

danc86 commented Aug 15, 2018

So lld doesn't understand this:

.heap _edata (INFO) :

but it understands this:

.heap (INFO) :

It should be redundant anyway, specifying the start address of each section to be the end address of the previous one. That will happen naturally anyway as the linker lays out each section one after the other. So I think we can just leave all the start addresses unspecified.

danc86 added 2 commits August 15, 2018 16:54
The .cfi_* assembler directives cause call frame information to be
emitted in a special .eh_frame section. But this is not needed in the
final binary, because we are not doing panic unwinding.

GNU ld already discards this section with its (default) --gc-sections
behaviour. Lld doesn't do that by default though, instead it complains:

    rust-lld: error: no memory region specified for section '.eh_frame'

So let's explicitly discard the .eh_frame section.
To work around a bug in lld, we need to avoid starting the .bss section
from a fixed address (previously, the _sbss symbol). Otherwise lld
incorrectly tries to extend the FLASH output region up to the start of
the RAM output region, which is too large.

To work around another bug in lld, we need to avoid starting sections
marked with (INFO) from a specific starting address. Its parser does not
accept both a start address and the (INFO) attribute.

Specifying these start addresses is redundant anyway, the linker will
lay them out in sequential order for us.
@danc86
Copy link
Contributor Author

danc86 commented Aug 15, 2018

Okay! This is finally ready to go now I think. :-)

I just pushed an amended series, changes are:

  • Keep the .cfi directives, just discard the .eh_frame section. GNU ld already discards it anyway, we don't need it at runtime.
  • Use (INFO) but drop all the redundant start addresses, to work around the various lld bugs.

With these patches, both GNU ld and lld can successfully link and produce working binaries. For comparison, this is what you get from GNU ld before these patches (current master):

Program Header:
    LOAD off    0x00001000 vaddr 0x20400000 paddr 0x20400000 align 2**12
         filesz 0x0000b3b8 memsz 0x0000b3b8 flags r-x
    LOAD off    0x0000d000 vaddr 0x80000000 paddr 0x80000000 align 2**12
         filesz 0x00000000 memsz 0x00000004 flags rw-
   STACK off    0x00000000 vaddr 0x00000000 paddr 0x00000000 align 2**4
         filesz 0x00000000 memsz 0x00000000 flags rw-

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         0000aa48  20400000  20400000  00001000  2**4
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .rodata       00000968  2040aa50  2040aa50  0000ba50  2**4
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  2 .bss          00000004  80000000  80000000  0000d000  2**2
                  ALLOC
  3 .data         00000000  80000004  80000004  0000c3b8  2**2
                  CONTENTS, ALLOC, LOAD, DATA
  4 .heap         00000000  80000004  80000004  0000c3b8  2**2
                  CONTENTS
  5 .stack        80004000  80000004  80000004  0000c3b8  2**2
                  CONTENTS
[...debug sections...]

This is what you get with GNU ld with these patches applied (note the layout is the same, nothing has regressed as far as I can tell):

Program Header:
    LOAD off    0x00001000 vaddr 0x20400000 paddr 0x20400000 align 2**12
         filesz 0x0000b3a8 memsz 0x0000b3a8 flags r-x
    LOAD off    0x0000d000 vaddr 0x80000000 paddr 0x80000000 align 2**12
         filesz 0x00000000 memsz 0x00000004 flags rw-
   STACK off    0x00000000 vaddr 0x00000000 paddr 0x00000000 align 2**4
         filesz 0x00000000 memsz 0x00000000 flags rw-

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         0000aa48  20400000  20400000  00001000  2**4
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .rodata       00000960  2040aa48  2040aa48  0000ba48  2**4
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  2 .bss          00000004  80000000  80000000  0000d000  2**0
                  ALLOC
  3 .data         00000000  80000004  80000004  0000c3a8  2**0
                  CONTENTS, ALLOC, LOAD, DATA
  4 .heap         00000000  80000004  80000004  0000c3a8  2**0
                  CONTENTS
  5 .stack        80004000  80000004  80000004  0000c3a8  2**0
                  CONTENTS
[...debug sections...]

And this is what you get using built-in rust-lld (7.0.0 + riscv patches):

Program Header:
    LOAD off    0x00001000 vaddr 0x20400000 paddr 0x20400000 align 2**12
         filesz 0x0000b3cc memsz 0x0000b3cc flags r-x
    LOAD off    0x0000d000 vaddr 0x80000000 paddr 0x80000000 align 2**12
         filesz 0x00000000 memsz 0x00000004 flags rw-
    LOAD off    0x0000d004 vaddr 0x80000004 paddr 0x2040b3cc align 2**12
         filesz 0x00000000 memsz 0x00003ffc flags rw-
   STACK off    0x00000000 vaddr 0x00000000 paddr 0x00000000 align 2**0
         filesz 0x00000000 memsz 0x00000000 flags rw-

Sections:
Idx Name          Size      VMA       LMA       File off  Algn
  0 .text         0000aa48  20400000  20400000  00001000  2**4
                  CONTENTS, ALLOC, LOAD, READONLY, CODE
  1 .rodata       0000096c  2040aa60  2040aa60  0000ba60  2**5
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  2 .bss          00000004  80000000  80000000  0000d000  2**0
                  ALLOC
  3 .data         00000000  80000004  80000004  0000d004  2**0
                  ALLOC
  4 .heap         00000000  80000004  80000004  0000d004  2**0
                  ALLOC
  5 .stack        00003ffc  80000004  2040b3cc  0000d004  2**0
                  ALLOC
[...debug sections...]

The lld version is still not perfect. You can see it treats the .stack section as allocatable in the output, and so there is an unnecessary load of size 0x00003ffc for populating the stack in the program header. It seems to be (yet another) bug in lld's implementation of (INFO).

But the main thing is the linker produces a binary, and the binary actually works! So I think that is good enough to merge for now.

I have requested an account on the LLVM Bugzilla so that I can file all these bugs with them.

@danc86 danc86 changed the title [WIP] support linking with lld support linking with lld Aug 15, 2018
@danc86
Copy link
Contributor Author

danc86 commented Aug 15, 2018

I'm not even sure what the .stack and .heap sections are needed for. Are they just informational only? The linker script in cortex-m-rt doesn't define them, which is how they avoid all these issues I guess.

@dvc94ch
Copy link
Member

dvc94ch commented Aug 15, 2018

I guess we can remove them. The idea was to be able to see the stack/heap size easily with readelf.

Are you planning on submitting the lld patch to rust-lld? Or would you prefer me to do it?

I'll need some time to read through all of this again. Takes some time for me to process since I'm not a linker script expert...

@danc86
Copy link
Contributor Author

danc86 commented Aug 16, 2018

Yeah take your time :-) I certainly had to learn a lot about linker stuff in the process of writing this PR!

We might as well keep the .stack and .heap for now. The extra load of empty memory seems to be harmless. I will file it as a bug against lld so maybe it will be fixed soon anyway.

I will post a PR against rust-lld for the RISCV patch too.

@dvc94ch
Copy link
Member

dvc94ch commented Aug 16, 2018

r+

Copy link
Member

@dvc94ch dvc94ch left a comment

Choose a reason for hiding this comment

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

r+

@dvc94ch
Copy link
Member

dvc94ch commented Aug 17, 2018

@bors r+

@dvc94ch
Copy link
Member

dvc94ch commented Aug 17, 2018

@homu r+

@dvc94ch
Copy link
Member

dvc94ch commented Aug 17, 2018

If someone could tell me how to accept this pr that would be nice xD

@danc86
Copy link
Contributor Author

danc86 commented Aug 17, 2018

Ha... I might be wrong but I think we are using bors-ng here... try bors r+ without the @.

@dvc94ch
Copy link
Member

dvc94ch commented Aug 18, 2018

bors r+

bors bot added a commit that referenced this pull request Aug 18, 2018
10: support linking with lld r=dvc94ch a=danc86

Rust now ships an embedded copy of lld, so if we can link crates with lld it will make the getting started experience way smoother (don't need to find a separate GNU ld).

Co-authored-by: Dan Callaghan <[email protected]>
@bors
Copy link
Contributor

bors bot commented Aug 18, 2018

Build succeeded

@bors bors bot merged commit 52095e9 into rust-embedded:master Aug 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants