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

App loading with more features #69

Closed
wants to merge 10 commits into from

Conversation

Ericson2314
Copy link
Contributor

  1. Relocations are performed, obviating the need for manual pic sprinkling.

  2. Globals with initial values (vs all-0 globals in BSS) are now initialized to those values.

Contains #68, though if that PR is not wanted this is easy to undo.

Intended benefits

- More parallelism

- Failures are clearer at a glance
1. Relocations are performed, obviating the need for manual `pic`
   sprinkling.

2. Globals with initial values (vs all-0 globals in BSS) are now
   initialized to those values.
@Ericson2314 Ericson2314 marked this pull request as ready for review February 14, 2023 14:12
@yhql
Copy link
Contributor

yhql commented Feb 27, 2023

Thanks for this landmark contribution!
I did merge #68.
Wondering about the use of link_wrap.sh, which would not work on Windows/Mac (might be a detail but it is nice to not have to worry about this anymore). This could be integrated into cargo-ledger for platform indepence, what do you think?
(I suspect you don't use cargo-ledger though 😄)

@Ericson2314
Copy link
Contributor Author

We actually do use cargo-ledger! Yeah I suppose it could be made part of that, good idea.

@yhql
Copy link
Contributor

yhql commented Mar 3, 2023

Ok, then this is a 'nice' option for cross-platform building, but would be really nice if that wasn't even necessary. I recall you mentioned trying to have a section containing relocations directly in the linker script does not work @Ericson2314 , because then it's also considered something to relocate and trouble ensues. Do you have a link to anything that mentions this problem? Even if it points into LLVM's source code, I'm ready 😄

…ss multiple runs

The `pic` API can only detect the linked code addresses, and in principle cannot
help if the app's flash storage has been moved after the relocations had been
already applied.
This fix does the relocations by storing the _nvram value and
comparing it across multiple app runs to detect changes.
@dfordivam
Copy link
Contributor

The problem with doing the "link-wrap" phase in the cargo ledger is that the cargo run and cargo test commands will not be able to work. And I am not sure invoking cargo-ledger from cargo test would be a good idea.

@yhql
Copy link
Contributor

yhql commented Apr 18, 2023

Yes agreed! So I just need to find a way to execute those commands in Windows properly, but having a linker field that can be different depending on the host does not appear to be possible.

Your PR is great, I'm just unclear on why we can't have the .rel.* sections in their own sections. I didn't find any information that would confirm that this is the intended behaviour, or a blindspot of lld.

@yogh333
Copy link
Contributor

yogh333 commented Mar 11, 2024

See #128

@yogh333 yogh333 closed this Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants