Generate unique id for device peripherals

[gephaistos]

Hi! I encoutered errors when compiling project with multiple devices used and lto enabled. The problem is similar to this issue with generated variable DEVICE_PERIPHERALS. This PR solves the problem by generating unique name based on the device name ~~removing #[no_mangle] attribute~~.

[jannic]

Not a complaint about your solution - AFAIKT it should work - but I wonder:

The comment above DEVICE_PERIPHERALS states:

        // NOTE `no_mangle` is used here to prevent linking different minor versions of the device
        // crate as that would let you `take` the device peripherals more than once (one per minor
        // version)

If you only get errors with LTO enabled, is the purpose stated in that comment reached at all? If it doesn't work anyway, we could as well remove the no_mangle and avoid the complexity of generating a unique name.

[Emilgardis]

If you only get errors with LTO enabled, is the purpose stated in that comment reached at all? If it doesn't work anyway, we could as well remove the no_mangle and avoid the complexity of generating a unique name.

That's certainly a bug though? The idea is that, if it works, you should not be able to easily claim two mutable references using two different versions of the library.

I think the no_mangle should stay and a name corresponding to the device name to be used, its still useful, and also useful even with non-mut static (in the future 2024 edition).

[Shatur]

I'm not sure if this is the intended way to use no_mangle.

[Emilgardis]

To me it seems like a fair way to use it, but maybe we could (ab)use package.links instead.

[Emilgardis]

no_mangle makes a symbol visible (unconditionally) under that exact name. Why ThinLTO allows this is very strange

[jannic]

no_mangle makes a symbol visible (unconditionally) under that exact name. Why ThinLTO allows this is very strange

https://github.com/rust-lang/rust/issues/28179 may be relevant here.

Money quote: "I'd argue that having multiply-defined #[no_mangle] symbols is, in essence, undefined behavior (in the sense that nobody defined it)"

[adamgreig]

We discussed this a bit in the meeting today.

• Renaming it from DEVICE_PERIPHERALS means that new versions of a PAC could link with old versions, which is unsound
• Same problem with swapping just to links in Cargo.toml
• I think as long as we provide the safely-acquired owned singletons, we have to keep this no_mangle DEVICE_PERIPHERALS, even though it kinda sucks. As far as I can tell it does usually prevent linking together.
• We could maybe add a runtime feature flag to disable acquiring owned singletons and thus also disable the DEVICE_PERIPHERALS static, which would allow two PACs to be linked together (but both would only provide unsafe APIs for obtaining the peripherals)

[jannic]

* I think as long as we provide the safely-acquired owned singletons, we have to keep this `no_mangle` `DEVICE_PERIPHERALS`, even though it kinda sucks. As far as I can tell it does _usually_ prevent linking together.

Just as a data point: I did some quick tests using rp2040-pac. Compiling a program calling Peripherals::take().unwrap() from both rp2040-pac 0.5.0 and 0.6.0 from main succeeds or fails depending on the lto settings:

• false (and codegen-units=2): fails with linker error rust-lld: error: duplicate symbol: DEVICE_PERIPHERALS
• off: fails with linker error rust-lld: error: duplicate symbol: DEVICE_PERIPHERALS
• thin: succeeds without warning
• fat: compiler warning warning: Linking globals named 'DEVICE_PERIPHERALS': symbol multiply defined! followed by compiler error error: failed to load bitcode of module "rp2040_pac-bff007a1c8be64d8.rp2040_pac.8071c7cd85106f05-cgu.0.rcgu.o"

This is the same for both release and debug builds.

[adamgreig]

Thanks, it's useful to know it's only a problem on thin LTO. Perhaps another good reason to plan to move away from owned singletons given out like this.

[jannic]

Related: https://github.com/rust-embedded/wg/issues/467