-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
document our assumptions about symbols provided by the libc #114412
Conversation
r? @thomcc (rustbot has picked a reviewer for you, use r? to override) |
One question is whether we should explicitly state that just because this library currently makes these assumptions, that does not grant Rust code the license to import Cc @Amanieu |
library/core/src/lib.rs
Outdated
//! `strlen`. Their signatures are the same as found in C, but there are extra assumptions about | ||
//! their semantics: For `memcpy`, `memmove`, `memcmp`, and `memset`, if the `n` parameter is 0, | ||
//! the function is assumed to not be UB. Furthermore, for `memcpy`, if source and target pointer | ||
//! are equal, the function is assumed to not be UB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth pointing out that these assumptions come from LLVM and are shared by GCC. Otherwise users could be surprised that Rust has such "strong requirements", but putting them into perspective with LLVM and GCC shows that it's really not that strong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The memcmp assumption is only ours, AFAIK.
Does GCC document their assumptions somewhere? For LLVM, should I link to that phabricator review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally it would link to the LLVM docs after those are merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the LLVM PR has been sitting for almost 3 years so I wouldn't want to wait for that.^^
With #114382 having landed, it is really only the compiler, not the standard library, that is making these assumptions. So maybe I should hand this to the compiler team for review? r? compiler |
@bors r+ rollup (at some point in my life I will write some text about "linking gotchas" that discusses some of the problems I had to diagnose that had the emitted calls to these functions as part of their root cause.) |
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#113510 (Document soundness of Integer -> Pointer -> Integer conversions in `const` contexts.) - rust-lang#114412 (document our assumptions about symbols provided by the libc) - rust-lang#114813 (explain why we can mutate the FPU control word) - rust-lang#115523 (improve `AttrTokenStream`) - rust-lang#115536 (interpret: make MemPlace, Place, Operand types private to the interpreter) - rust-lang#115540 (Support debuginfo for custom MIR.) - rust-lang#115563 (llvm-wrapper: adapt for LLVM API change) r? `@ghost` `@rustbot` modify labels: rollup
LLVM makes assumptions about
memcmp
,memmove
, andmemset
that go beyond what the C standard guarantees -- see https://reviews.llvm.org/D86993. Since we use LLVM, we are inheriting these assumptions.With #114382 we are also making a similar assumption about
memcmp
, so I added that to the list.Fixes rust-lang/unsafe-code-guidelines#426.