Remove implicit 'mem' feature for '-none' targets.

[MabezDev]

Now that rust-lang/rust#77284 is merged, anyone who needs the mem feature can opt-in with -Zbuild-std-features=compiler-builtins-mem. This change stops targets with '-none' in there triple from implicitly having the mem feature enabled.

Fixes #369

[josephlr]

Looks good to me, the -Zbuild-std-features=compiler-builtins-mem seems to work for https://github.com/cloud-hypervisor/rust-hypervisor-firmware on the latest nightly.

[alexcrichton]

I'm a little worried that this can break folks, isn't it conventional that "-none" targets don't have any external code? If external code is present could the target be named something else?

[MabezDev]

By external code, do you mean something like libc?

In my situation, I'm not linking against libc, or any other external code; I would argue my target is still a "-none" one. For the Xtensa LX6 chip I am developing for, some segments of memory can only be accessed in a word aligned manner. It's normal memory, but internally the bus used to access it has these restrictions. The compiler is free to insert these mem functions wherever it see's fit, hence the mem functions for this target have to work anywhere.

In terms of breakage, the decision to implicitly enable the"mem" feature is quite recent, in fact the PR describes itself as a bit of cludge to cover as many bases as possible. I understand not wanting to break peoples builds, but in my experience, opting in to cargo features is easy, opting out is hard.

Perhaps an alternative is to add my custom mem functions in this crate, behind a cfg target gate? Although, my target is still in the process of llvm upstreaming, and a long way away being an experimental target in Rust, so I am not sure this is the best idea.

[josephlr]

I'm a little worried that this can break folks, isn't it conventional that "-none" targets don't have any external code? If external code is present could the target be named something else?

I think as @MabezDev mentioned using string comparison against the target name to determine "is an external implementation of memcpy present" is a bit error-prone and confusing for custom targets. Also, some users of a target like aarch64-unknown-none want to provide their own memcpy, while others do not.

Due to how the target features work, if mem is on by default, it cannot be disabled. However, if it is off by default, users can enable it with -Zbuild-std-features=compiler-builtins-mem. This change seems like the best way forward.

[andre-richter]

This will be transparent to users of say aarch64-unknown-none that don't use cargo xbuild or cargo's build-std feature, right?
They will get the memcpy of the precompiled core-lib without needing to flip any switches (as it is today)?

[josephlr]

This will be transparent to users of say aarch64-unknown-none that don't use cargo xbuild or cargo's build-std feature, right?

So this should only matter for -Zbuild-std users, as cargo xbuild has a separate mechanism for enabling the mem feature of compiler-builtins.

They will get the memcpy of the precompiled core-lib without needing to flip any switches (as it is today)?

I'm assuming so, given that the change that enabled this only did so recently. So it looks like the prebuilt libcore for aarch64-unknown-none (and others) manually sets the mem feature (although I couldn't find the source that actually does this).

[alexcrichton]

@ehuss do you have thoughts on this perhaps as the author of #357?

Breakage is fine in my opinion I just want to make sure it's explicitly known and we weigh the various pros and cons.

[ehuss]

I'm a little concerned that I think this will essentially break everyone using build-std with a -none platform. Using -Zbuild-std-features seems like a reasonable short-term workaround, but I don't think it is a good long-term solution. rust-lang/wg-cargo-std-aware#53 (comment) has some interesting thoughts about using weak-linkage, but I'm not familiar enough with that to know how feasible that is. Another alternative is to have the desired behavior to be part of the Target spec, but I'm not sure if that makes sense or how exactly that should be exposed.

Regardless, I guess this should be OK, but it will likely cause some disruption.

[alexcrichton]

I think it should be fine to declare these symbols as weak by default. I don't think that would break anyone and I think it would fix the issue of users wanting to bring their own? (or libc is linked in for one reason or another)

[MabezDev]

Apologies, this dropped of my radar!

I think it should be fine to declare these symbols as weak by default.

I think this would work for me, and in fact is probably a better solution overall. I am happy to open a new PR with the changes and close this one?

One thing I struggled with before, is how to test these compiler-builtins changes? I tried using patch in the Cargo.toml and I don't remember managing to get it working.

[MabezDev]

I understand these changes are not to be made lightly, and will require some thought. Perhaps in the meantime would you be open to accepting a PR that adds the custom mem routines for the xtensa arch behind a cfg? Much like the current x86_64 optmized routines.

[Hecatron]

I recently run into this problem with the esp32
Also it doesn't look as if currently it's possible to use a custom version of compiler_builtins with build-std

• Can we tailor the version of the compiler-builtins package that is compiled and linked together with the core crate? wg-cargo-std-aware#61

[Amanieu]

As per #379 (comment), we should change the mem intrinsics to use weak linkage, which should resolve this issue. I'm going to close this PR, please open another one for weak linkage support.