add weak memcpy et al symbols
#29
Conversation
| @@ -0,0 +1,60 @@ | |||
| // NOTE Copied verbatim from the rlibc crate | |||
There was a problem hiding this comment.
May not be the best idea. There's a bug for all of these when n > ISIZE_MAX that I've been trying to figure out how to fix to submit a PR.
There was a problem hiding this comment.
Could you elaborate on what the bug is? Is it present on all architectures or only on AVR where usize is 16-bits(?).
There was a problem hiding this comment.
.offset takes an isize, which is limited to ISIZE_MAX
There was a problem hiding this comment.
Right, so if usize is a u8, then isize is an i8 with a maximum value of 127, and a value for n of 200 will do Bad Things. Scale up as appropriate for the platform.
Is there a possibility we could add the weak linking to rlibc and only depend on it for the appropriate platform(s)? I'm a big fan of laser-focused crates.
There was a problem hiding this comment.
The focus of this crate is to provide all of the runtime functions that are required to run Rust with libcore. This includes both compiler-rt builtins and a few libc functions (memset, memmove, memcpy, memcmp, fmod, fmodf).
There was a problem hiding this comment.
The focus of this crate is to provide all of the runtime functions
Sure, but couldn't that be accomplished by this crate using rlibc as a dependency on the platforms that need it? Then using this crate will pull in rlibc and it looks the same as if the code were implemented here?
There was a problem hiding this comment.
I don't feel too strongly about it either way. The point of making these symbols weak is that it allows you to override them with your own implementations if you want.
There was a problem hiding this comment.
Given that the ultimate goal is to have this crate as part of rust-lang/rust releases. It makes sense to me to also provide these mem* functions so users don't have to reach out for an external (crates.io) crate.
There was a problem hiding this comment.
Ok, I just really feel like it's a bad idea to copy-paste code like this, especially as we know there are bugs that will now need to get fixed in multiple locations. It seems like there should be some way to use rlibc without forcing a crates.io download, presumably similar to whatever will happen to this crate.
There was a problem hiding this comment.
I think I missed your point before. It makes sense to put rlibc below this crate to have to fix bugs in a single place. To do that rlibc would have to gain a "weak" Cargo feature that marks the mem* functions as weak symbols.
there should be some way to use rlibc without forcing a crates.io download
Probably just make it a submodule of rust-lang/rust. Like this crate/repository will.
|
Does this work on Windows/Mac? Is memcpy getting properly overridden if one is provided by libc? |
I recall that WindowsBunny reported that
I haven't checked if they do get overridden when linked against libc. I'll test the overrides using a |
|
I think we can leave out these functions on Windows/Mac since it's likely you'll be a) running with |
| fn memmove(dest: *mut u8, src: *const u8, n: usize) -> *mut u8; | ||
| fn memset(dest: *mut u8, c: i32, n: usize) -> *mut u8; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it is better to leave this as it was since the memcpy symbol might be overridden.
|
Also amusing: alexcrichton/rlibc#11 |
|
Updated this to use the rlibc crate. |
|
We are now using rlibc so I'm going to go ahead and merge this. The upstream LTO fix has been approved and it's already in the homu queue. |
closes #28
rust-lang/rust#35637 needs to land first
r? @Amanieu
cc @shepmaster