rust-embedded / cortex-m-rt

Minimal startup / runtime for Cortex-M microcontrollers
https://rust-embedded.github.io/cortex-m-rt/
Apache License 2.0
358 stars 85 forks source link

Replace __ONCE__ with Cargo links key #276

Closed adamgreig closed 4 years ago

adamgreig commented 4 years ago

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

rust-highfive commented 4 years ago

r? @jonas-schievink

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

jonas-schievink commented 4 years ago

:+1: 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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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.

bors[bot] commented 4 years ago

Build succeeded:

therealprof commented 4 years ago

I guess we should release this ASAP then?

adamgreig commented 4 years ago

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 commented 4 years ago

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

adamgreig commented 4 years ago

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 commented 4 years ago

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.