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

compiler_builtins seems to be missing symbols #53

Open
tomaka opened this issue Jan 20, 2020 · 19 comments · Fixed by rust-lang/rust#77284
Open

compiler_builtins seems to be missing symbols #53

tomaka opened this issue Jan 20, 2020 · 19 comments · Fixed by rust-lang/rust#77284
Labels
bug Something that isn't working as expected S-needs-design Status: needs design work stabilization blocker This needs a resolution before stabilization

Comments

@tomaka
Copy link

tomaka commented Jan 20, 2020

This is the continuation of rust-lang/compiler-builtins#334

When compiling a pure Rust crate with -Zbuild-std=core,alloc, I get missing symbols errors for everything related to memcpying (memcpy, memset).
This is covered by the mem feature of compiler_builtins, which for a reason I don't understand isn't enabled by default.

Until mid-December can be solved by explicitly depending on compilter_builtins with the mem feature. However, one of the nightlies broke this and when doing so one now gets:

multiple rlib candidates for compiler_builtins found

I believe that the original cause is the features not enabled on compiler_builtins.

@phil-opp
Copy link

One workaround is to add a dependency on rlibc, which provides these symbols. You might need to add an extern crate rlibc; statement to ensure that it is linked.

@ehuss
Copy link
Contributor

ehuss commented Jan 22, 2020

@alexcrichton can you verify — the reason the mem feature is not needed for std is because those symbols are exported from libc?

That's a serious complication for building std on platforms without libc. 😞

@alexcrichton
Copy link
Member

Ah yes, this is something we'll need to handle. Most platforms don't enable the mem feature of compiler-builtins because it's not necessary and the platform libc typically has better optimized routines than what's implemented in compiler-builtins. The build system however sometimes enables mem, notably for no_std targets, where typically there is no other platform libc.

How Cargo would know what to do here is... unknown. Although arguably if we "fix" target-specific features we could have target-specific dependencies on compiler-builtins which enable the mem feature for only a whitelist of targets, so this could theoretically be fixed with your work @ehuss.

@cr1901
Copy link

cr1901 commented Apr 27, 2020

This is relevant for the msp430 target, FWIW.

Two solutions are to bring in libc as a dependency (-Clink-arg=-lgcc) or link against rlibc as @phil-opp mentions (the libc solution being about 400 bytes savings in debug mode b/c well, libc is optimized :P).

However, when I do debug builds with -Zbuild-std=core, with using the "link to lib C" solution, I get some worrying (to me) warnings when certain conditions are met (#55).

@ehuss
Copy link
Contributor

ehuss commented Jun 5, 2020

Note: there are two semi-related things that need some investigation:

  • Supporting JSON spec targets. compiler-builtins (and several other crates) do sniffing based on the target name, which obviously won't work with JSON spec targets.
  • error adding symbols: file format not recognized cargo#8239 gives an example where compiler-builtins symbols are not found when using ld. I did some experimenting, and with the mem feature enabled, I get undefined symbols from memcpy (and friends) to various things in core. I'm not really sure why that is.

@MabezDev
Copy link

I am building for a no_std platform, where I have to supply the mem functions (all flash access must be word aligned), unfortunately with the merge of rust-lang/compiler-builtins#357 I can't find a way to opt out of the mem option, other than renaming my target not to include -none.

Any suggestions?

Are there any longer term plans to allow the build-std feature to customize some of the crates it builds?

@josephlr
Copy link

Are there any longer term plans to allow the build-std feature to customize some of the crates it builds?

The -Zbuild-std-features unstable flag exists (see rust-lang/cargo#8490), which allows users to set features when using -Zbuild-std. However, I don't think the mem feature is wired up correctly. I'll take a look and see what I find out.

@DianaNites
Copy link

@josephlr I think it'd Just Work if std had a compiler-builtins-mem = ["alloc/compiler-builtins-mem"] feature in Cargo.toml, since alloc already exposes the feature, but it's unusable from build-std-features

@toothbrush7777777
Copy link

@DianaNites Will that work when using -Zbuild-std=core?

@DianaNites
Copy link

@toothbrush7777777 I don't see why it would? If you're asking for just core to be built, why would it build compiler_builtins?

@toothbrush7777777
Copy link

@DianaNites I read somewhere that core built compiler_builtins. Maybe that is wrong. Anyway, it would be nice to be able to have the mem feature of compiler_builtins enabled without building alloc.

@Lokathor
Copy link

It's also my understanding that core relies on compiler_builtins as part of the build.

@DianaNites
Copy link

Ah yeah requiring alloc, good point. Though idk if it actually does? build-std is weird, and build-std-features even weirder, they're the features of std right? but whose using build-std to build.. std?

core doesn't list any dependencies, so idk. Might it just rely on the symbols compiler_builtins provides, during linking? alloc depends on both core and compiler_builtins, bringing it in, though std depends on all 3 so..

@josephlr
Copy link

@josephlr I think it'd Just Work if std had a compiler-builtins-mem = ["alloc/compiler-builtins-mem"] feature in Cargo.toml, since alloc already exposes the feature, but it's unusable from build-std-features

This is essentially correct. Right now, feature resolution is done though the test crate regardless of what crates you actually end up building. This means that the mem feature needs to be forwarded from alloc, to std, to test. This is similar to what other features already do.

@DianaNites I read somewhere that core built compiler_builtins. Maybe that is wrong. Anyway, it would be nice to be able to have the mem feature of compiler_builtins enabled without building alloc.

You're correct, building core will automatically build compiler_builtins, similar to how std automatically builds all of the crates std depends on.

Ah yeah requiring alloc, good point. Though idk if it actually does? build-std is weird, and build-std-features even weirder,
they're the features of std right? but whose using build-std to build.. std?

This (building core with the mem feature on) is my goal as well. Due to the resolution hack linked above, once we forward things correctly, the mem feature should work even if you're only depending on core and compiler-builtins.

core doesn't list any dependencies, so idk. Might it just rely on the symbols compiler_builtins provides, during linking?

This is correct. core (and any Rust code generated by LLVM) depends on the symbols in compiler_builtins to link correctly. compiler_builtins depends on core as a "normal" Rust dependency. This circularity always struck me as weird, but it's necessary to deal with using non-Rust compiler-rt.

josephlr added a commit to josephlr/rust that referenced this issue Sep 28, 2020
This fixes rust-lang/wg-cargo-std-aware#53

Now users will be able to do:
```
cargo build -Zbuild-std=core -Zbuild-std-features=compiler-builtins-mem
```
and correctly get the Rust implemenations for `memcpy` and friends.

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link

josephlr commented Oct 5, 2020

@tomaka @alexcrichton this can be closed now that rust-lang/rust#77284 is merged.

@ehuss
Copy link
Contributor

ehuss commented Oct 5, 2020

@josephlr I wouldn't consider that a complete fix for this issue, as it appears to require manually setting the compiler-builtins-mem feature, is that correct? I would prefer to not force the user to need to know about internal feature names.

@josephlr
Copy link

josephlr commented Oct 5, 2020

@josephlr I wouldn't consider that a complete fix for this issue, as it appears to require manually setting the compiler-builtins-mem feature, is that correct? I would prefer to not force the user to need to know about internal feature names.

If you're building for a custom target, you will have to tell cargo whether or not your platform provides implementations of memcpy or not, so there not much more we could do here. There's going to have to be a build-time switch.

@josephlr
Copy link

josephlr commented Oct 6, 2020

Actually, @ehuss I think you might be on to something here. In an ideal world, users would be able to override memcpy, but if they didn't we would fallback to compiler_builtins::memcpy automatically. Users shouldn't have to deal with this stuff directly. The solution to this is weak linkage (see the Rust tracking issue).

Here's how I think this all should work long-term:

  • compiler-builtins no longer has a mem feature
  • Instead the memcpy functions are declared in compiler-builtins with #[linkage = "weak"]
  • If a user manually provides a "strong" version of memcpy (or are on a platform like Linux that does it for them). That version is automatically used
  • Otherwise, the "weak" fallback implementation is used.

This would be the idea state of affairs in my opinion, but it probably requires some subset of rust-lang/rust#29603 to be on a path to stabilization before we can use it. Note that compiler-builtins currently uses weak linkage for some arm targets (see rust-lang/compiler-builtins#378), but memcpy and friends are used on essentially every target, so there would need to be some way for weak linkage to work on all targets/linkers.

This might be possible in the future, but probably can't be done right now.

AxlLind added a commit to AxlLind/AxOS that referenced this issue Oct 6, 2020
This issue made it impossible to use -Zbuild-std without implementing
the memory related symbols required by the core lib such as memcpy,
memset, etc yourself.

rust-lang/wg-cargo-std-aware#53

After this PR you can now tell cargo that your platform supports these
mem features and just use the built in implementations in rustc.
@MabezDev
Copy link

FYI, I submitted rust-lang/compiler-builtins#411 a few years ago and it's been possible to override the memcpy and family of functions downstream ever since.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something that isn't working as expected S-needs-design Status: needs design work stabilization blocker This needs a resolution before stabilization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants