8000 Fix something that was not the root cause of relocation bug from #2219 at the time, the root cause was a bug in LLD, and now that bug is fixed, so we can use the canonical way of ensuring the relocation section has a predictable LMA by alevy · Pull Request #4108 · tock/tock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix something that was not the root cause of relocation bug from #2219 at the time, the root cause was a bug in LLD, and now that bug is fixed, so we can use the canonical way of ensuring the relocation section has a predictable LMA #4108

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 17, 2024
Merged

Fix something that was not the root cause of relocation bug from #2219 at the time, the root cause was a bug in LLD, and now that bug is fixed, so we can use the canonical way of ensuring the relocation section has a predictable LMA #4108

merged 1 commit into from
Jul 17, 2024

Conversation

alevy
Copy link
Member
@alevy alevy commented Jul 17, 2024

Pull Request Overview

This pull request fixes the actual root cause of #2219 and fixed originally in #2336

Currently we rely on -nmagic for the linker not to do funny things with the beginning of the relocation section such that it always actually is stored starting at _etext. This is a hack. Instead, we can force the load-memory-address (LMA) of the relocate section to be exactly _etext using AT (_etext).

see https://sourceware.org/binutils/docs/ld/Output-Section-LMA.html

Testing Strategy

This is compile tested. In practice, this doesn't actually remove -nmagic since that's still nice to have (the linker won't inject zero padding unnecessarily to the binary).

I validated that this precisely places the relocation section by removing -nmagic, and also by modifying the AT (_etext) statement to be various offsets from _etext and manually inspecting the binary.

TODO or Help Wanted

N/A

Documentation Updated

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

Formatting

  • Ran make prepush.

@ppannuto
Copy link
Member

I think this missed a file in git. Only the makefile change is here (though, the PR description certainly makes sense for what should also be here I think)

@bradjc
Copy link
Contributor
bradjc commented Jul 17, 2024

This is frustrating because we are just going in circles. I don't trust the LLVM linker. #2816

In the past we changed this because things broke. Now we are changing it because it seems wrong.

@alevy
boards: linker: fix root cause of relocation bug
334d7af 
Currently we rely on `-nmagic` for the linker not to do funny things
with the beginning of the relocation section such that it always
actually is stored starting at `_etext`. This is a hack. Instead, we can
force the load-memory-address (LMA) of the relocate section to be
exactly `_etext` using `AT (_etext)`.

see <https://sourceware.org/binutils/docs/ld/Output-Section-LMA.html>
@alevy alevy force-pushed the fix-nmagic-root-cause branch from 69dab0c to 334d7af Compare July 17, 2024 16:25
@alevy
Copy link
Member Author
alevy commented Jul 17, 2024

@ppannuto and @bradjc please look again as though you hadn't seen the original diff. I committed the wrong file and didn't commit the right file originally, so what you saw is the opposite of the purpose. This is now correct as intended.

@bradjc
Copy link
Contributor
bradjc commented Jul 17, 2024

Is the premise of this PR that the LLVM linker is fixed? This PR is really just reverting e17d4ac

@alevy
Copy link
Member Author
alevy commented Jul 17, 2024

Is the premise of this PR that the LLVM linker is fixed

This does seem to be explicitly supported in ld.lld: https://releases.llvm.org/11.0.1/tools/lld/docs/ELF/linker_script.html#output-section-lma . I can't find since which version.

As far as I can tell this was a bug in ARM targets (https://bugs.llvm.org/show_bug.cgi?id=45313 https://bugs.llvm.org/show_bug.cgi?id=38776) that was fixed (llvm/llvm-project@bb4a36e).

@alevy
Copy link
Member Author
alevy commented Jul 17, 2024

This PR is really just reverting e17d4ac

Technically reverting except retaining the whitespace fixes :)

@ppannuto
Copy link
Member

Man, we almost didn't have to deal with this bug; latency of rust pulling llvm releases I imagine :/.

Looks like a reasonable thing to revert assuming nothing else changes. I imagine we could just compile all the boards on both sides of this patch and diff the output of readelf for everything—no change would mean no problem for any board?

@bradjc
Copy link
Contributor
bradjc commented Jul 17, 2024

I'm ok with this if we change the PR title. This was in the tree at the time of the issue

tock/boards/kernel_layout.ld

Line 255 in 7c2bf67

.relocate : AT (_etext)

yet, well, here we are.

A lot of debugging went into making the linker work and making Tock without additional dependencies. It's not as simple as just saying one line of a linker script was the root cause.

@alevy alevy changed the title Fix root cause of relocation bug from #2219 Fix something that was not the root cause of relocation bug from #2219 at the time, the root cause was a bug in LLD, and now that bug is fixed, so we can use the cannonical way of ensuring the relocation section has a predictable LMA Jul 17, 2024
@alevy
Copy link
Member Author
alevy commented Jul 17, 2024

@bradjc New title, tell me what you think. Feel free to adjust however you like

@alevy alevy mentioned this pull request Jul 17, 2024
Merged
5 tasks
@lschuermann lschuermann changed the title Fix something that was not the root cause of relocation bug from #2219 at the time, the root cause was a bug in LLD, and now that bug is fixed, so we can use the cannonical way of ensuring the relocation section has a predictable LMA Fix something that was not the root cause of relocation bug from #2219 at the time, the root cause was a bug in LLD, and now that bug is fixed, so we can use the canonical way of ensuring the relocation section has a predictable LMA Jul 17, 2024
@alevy alevy added the last-call Final review period for a pull request. label Jul 17, 2024
@ppannuto ppannuto added this pull request to the merge queue Jul 17, 2024
Merged via the queue into tock:master with commit 31f299d Jul 17, 2024
12 checks passed
@alevy alevy deleted the fix-nmagic-root-cause branch July 17, 2024 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppannuto ppannuto ppannuto approved these changes

@bradjc bradjc bradjc approved these changes

@lschuermann lschuermann lschuermann approved these changes

Assignees
No one assigned
Labels
last-call Final review period for a pull request.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alevy @ppannuto @bradjc @lschuermann
0