Skip to content
This repository has been archived by the owner on Jan 24, 2022. It is now read-only.

Replace __ONCE__ with Cargo links key #276

Merged
merged 2 commits into from
Jul 9, 2020
Merged

Replace __ONCE__ with Cargo links key #276

merged 2 commits into from
Jul 9, 2020

Conversation

adamgreig
Copy link
Member

@adamgreig adamgreig commented Jul 3, 2020

This would fix #275. We use the links key in cortex-m already here. See also rust-embedded/wg#467.

@rust-highfive
Copy link

r? @jonas-schievink

(rust_highfive has picked a reviewer for you, use r? to override)

@jonas-schievink
Copy link
Contributor

👍 to adding links, all rt and peripheral crates should have that. When I added it to cortex-m though I left the static in place, since that's still needed to prevent linking with older versions. If we don't do that we effectively lose the duplicate detection between newer and older cmrt versions.

@adamgreig
Copy link
Member Author

In cortex-m, the static you're talking about is CORE_PERIPHERALS: bool, right? Don't we need that static irregardless of linkage, so at most we could have removed the no_mangle?

The issue here is that because the static is zero sized, disassemblers mix it up with whatever other statics end up at the same address.

Nevertheless you're totally right and removing the static in the same release as adding the links key does stop duplicate detection between versions which is a pain. Any other ideas for fixing the debug issue? Making the static non-zero-sized seems undesirable.

@jonas-schievink
Copy link
Contributor

Ah, I see, yeah. That static has another purpose and need to be kept. I don't immediately see how to fix this debugging issue other than fixing the debug tooling (which arguably is the "proper" fix).

@adamgreig
Copy link
Member Author

@jonas-schievink I've updated to keep ONCE (though added a comment about this) and still add the links key, which I guess i the best we can do for now.

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

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

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 9, 2020

Build succeeded:

@bors bors bot merged commit 4d60835 into master Jul 9, 2020
@bors bors bot deleted the links-key branch July 9, 2020 10:26
@therealprof
Copy link
Contributor

I guess we should release this ASAP then?

@adamgreig
Copy link
Member Author

adamgreig commented Jul 9, 2020

I don't think we'll want to remove __ONCE__ for at least one or two minor/major releases, so there's probably no specific rush, but we probably do want a new c-m-rt patch release soonish.

@therealprof
Copy link
Contributor

Yes, a patch release to get "links" out into the field.

@adamgreig
Copy link
Member Author

Sure, it's worth doing eventually, but there's no rush since it won't make any difference until there's two releases using it (which could then be detected attempting to link together).

@therealprof
Copy link
Contributor

Maybe I'm a bit over-cautious but I'm wary about any new problems which could occur with the links approach in our context and just haven't been detected yet. AFAIK links has only/mostly been used for sys crates so far.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"error: cortex-m-rt appears more than once in the dependency graph"
4 participants