-
Notifications
You must be signed in to change notification settings - Fork 17
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
RFC for const-UB #58
RFC for const-UB #58
Conversation
In theory, C++ constexpr requires a compiler diagnostic when undefined behavior is executed. But this is somewhat easier in C++’s case due to differences in feature sets:
Also, from an implementation perspective, in the compilers I’ve looked at, GCC and Clang, the constexpr interpreter operates directly on the AST, so there aren’t any optimizations that could potentially remove UB. |
C++ has strict aliasing, which is somewhat comparable. But that gets us to your second point.
So there is no pointer arithmetic either? And what about unions? EDIT: Hm, for unions I guess they will just remember the active variant, then this is also safe... some union-based type punning is legal though, does constexpr support that? |
Wow, I had not expected this. Now I have second thoughts about this PR, do we really want to have fewer things checked than C++?^^ |
d9c2a11
to
96df19a
Compare
That's a weird way to formulate it imo. We are checking more things than C++, we do have safe code after all. What we are introducing to CTFE in this RFC is the One thing I think this RFC should mention is that we already have the policy that we may break unsound code in arbitrary ways, so if you do UB during CTFE we may start hard erroring on it at any point in the future.
I was under the impression that clang is converting the AST to a special datastructure similar to MIR which it then executes (but it used to run on AST until like a year or two ago). |
No, it doesn't support it.
There is pointer arithmetic, but only to move between elements of an array. You can't go out of bounds even within the same allocated object (e.g. moving between fields of a struct). (Sidenote: Both union-based type punning and traversing structs with pointer arithmetic fall under the category of "technically UB at runtime too, according to the C++ spec, but code depends on it and compilers allow it". Isn't C++ great? But for constexpr evaluation the rules are strictly enforced.)
Well, there is a new bytecode interpreter, which I was surprised just now to learn has landed in trunk already, but only under a flag, |
Anyway, going back to this RFC, I wonder if you could go into a little more detail about what sort of UB Miri cannot detect and how hard it would be to let it detect it in the future. After all, a potential compromise might be: "UB is not reliably detected for now, but we attempt to detect as much as possible, and we expect we will be able to detect all of it Someday™️– once the aliasing model has been nailed down and Miri has been improved." Normatively speaking that's not very different from the existing proposal, but it would suggest a different roadmap. It would also imply that Miri should never run optimizations capable of hiding UB when doing constant evaluation (at least not by default). |
Ah, so union-based type punning was a C thing then? It is definitely allowed in at least one of these languages...
Do you mean "Miri the tool" or the core Miri engine that powers CTFE? I don't think Miri the tool is very relevant for the discussion here; the RFC is about what CTFE does with the core Miri engine. Miri the tool already sets For the CTFE engine, the hardest part is running CTFE on unoptimized MIR even when the program is otherwise running with optimizations. Optimizations can have arbitrary effects on UB programs, so the answer to "which UB is not detected by the Miri engine during CTFE" is "potentially all of it". IIRC @oli-obk said during a meeting that making CTFE run on unoptimized MIR would be really hard to do. If we ignore the optimization problem, the UB that can slip through is mostly due to the semantics not being known yet: aliasing violations, validity violations, things around integer-pointer casts and pointer provenance. Some of this is not just tricky to figure out, it will also have major performance impact on CTFE. As I mentioned, we currently also ignore alignment, but that should not be hard to fix if we want to (Cc rust-lang/rust#63085).
All C++ code is |
Yep, it's in standard C but not C++. (The other feature, pointer arithmetic past the bounds of an array, is not defined in either language's spec... even though both standards define |
It's not hard, just expensive, very much so. We'd be holding two copies of all MIR of all
Well yes 🤣 I stated the same thing. My point was that all Rust code is
Well... Rust would then also be required to detect UB in safe code, as the unsafe code may by itself not cause any problems, but leave the safe parts in a state that they will. But this is getting into "what is even unsafe"-discussions, so let's table it.
Yes, if you formulate it that way, then I agree, but, see below...
I can easily construct a type which has never UB (or even unsafe code) at CTFE time but any runtime code touching it will blow up badly: pub struct Foo(*const u32);
pub const FOO: Foo(42);
impl Foo {
pub fn bar(&self) -> u32 {
unsafe {
*self.0
}
}
} So... I don't see the enormous advantage of detecting all CTFE UB, if it would still be trivial to create an object during CTFE that violates invariants that runtime depends upon. Yes we can do that for builtin types, and we do that mostly, but it's impossible to do for anything beyond that without introducing a whole invariant declaration system. I mean... what we're talking about here is unsafe/UB in So... Summarizing: You either uphold all your safety variants, even in safe code that has access to things that unsafe code relies on, or you wrote code that is UB. No compile-time unsafe code required at all to cause runtime UB. |
I don't follow your argument, @oli-obk. Sure there are "bugs that have something to do with const values" that the C++ approach does not catch. But there is an entire class of bugs that the C++ approach does catch, and it involves doing things at CTFE time like out-of-bounds I really don't understand how you are trying to frame things in a way here as if this was not a case of Rust missing bugs that C++ would catch.
Everything in the RFC and my discussion revers to any const-evaluation. It does not matter whether the offending code is directly in the "root" of the evaluated item, or inside some |
Maybe I should give an example: const CONST_UB: i8 = unsafe { 200.unchecked_add(100) }; Equivalent C++ code: constexpr signed char CONST_UB = 200 + 100; C++ is required to catch the UB (yeah I know integer promotion means the addition actually happens at a type where it does not overflow but you get the point). Rust with this RFC is not. Thus, Rust misses a bug that C++ would be required to catch. This worries me. The (obvious) fact that there are other bugs that Rust catches and C++ does not, and yet other bugs that neither catch (like your raw ptr deref example), is besides the point. |
I'm not trying to say that. I'm trying to say that it doesn't matter (to me? I was under the impression that it didn't matter to anyone that much, but apparently I was very wrong). We're currently preventing things that the CTFE engine can't handle, and that's enough. The RFC and the T-lang meeting both discuss(ed) how it's too expensive to do a full miri during CTFE and even then there'd still be some very rare and hard UB cases left. The alternative that C++ chose is to just prevent all kinds of operations, which, if we did the same, would severely limit what we can do at const eval time. I don't see us gaining many more improvements to what we have now (like heap allocations, string processing, ...) without unsafe code. I don't see limiting ourselves in the C++ way as a reasonable alternative. I don't even know if the C++ way was chosen out of safety reasons or just to not put a full VM requirement onto compiler developers. If I'd had to put my money anywhere it would be the latter. I also don't know if C++ will keep this scheme or become more liberal in the future.
We'd be breaking a lot of code out there if we suddenly banned everything we can't prove sound. It's already stable to do unsafe things in static and const initializers. So afaict we're really just talking about const fn, even if the wording is independent of the const context. As you said, CTFE is "only" missing aliasing and validity checks from miri, so if we want to get very pedantic, we can just say that we catch a certain list of UB. If we want to keep comparing with C++ I read https://shafik.github.io/c++/undefined%20behavior/2019/05/11/explporing_undefined_behavior_using_constexpr.html a while back and it shows that while obvious UB is detected, there's lots of UB like ordering issues, enum discriminants, ... Quoting from the article:
So while the C++ standard requires absolute freedom from UB, this does not hold in practice due to it being too expensive. Anyway, my personal opinion is to keep doing a best effort UB detection during CTFE. I am very bad at articulating the concrete reasons. I think my feelings on the discussion on this RFC is best summarized as
|
I agree that once CTFE supports (almost) all of Rust, requiring it to detect all UB is asking too much, and indeed that is not the precedent C++ is setting. This would require "full Miri" and I'd be fine with not doing that. But at the design meeting, I didn't know about what C++ does, and there are more options between "full Miri" and what this RFC proposes, options that were not considered at the design meeting. Specifically, I think it would be reasonable to expect that for the fragment of operations that C++ supports, Rust is also able to detect UB during CTFE. So basically, if a CTFE evaluation does not use raw pointers, union, or In other words, before rejecting the idea, I think we should figure out how expensive it really is to run CTFE on unoptimized MIR.
I am very confused now. Nowhere did I propose to "ban everything we cannot prove sound", so why are you bringing up that proposal now? Everything in the RFC is about dynamic checks, not static ones! I thought that was obvious as it was talking about
We don't, though. We run after optimizations. Any UB that optimizations exploit, we cannot detect any more reliably when running the optimized code. This is a fundamental impossibility that also affects tools like e.g. valgrind.
We did not discuss this to death at all I think, this is the first serous discussion about it since the design meeting and before that we didn't talk much about it either. (We talked about "unconst" quite a bit, but that is a different discussion. Also we didn't really reach a conclusion or even properly structure the design space, so I don't think that is "discussed to death" either.) New information arose since the design meeting, which is sufficient justification to re-consider some of the conclusions we made before we knew about that information. |
I entirely misunderstood that you were in fact proposing
which is very reasonable, and we're (mostly?) doing already anyway. So other than const evaluating unoptimized MIR, we'll have to vet all intrinsics we stabilize to make sure they capture all UB that does not require pointer magics to cause.
The problem is that we can't really gauge this yet, as only a small percentage of functions is
I did absolutely overreact here, and then doubled down by defending that position, I apologize. We should definitely address any concerns that are brought up. |
[reference-level-explanation]: #reference-level-explanation | ||
|
||
When UB arises as part of CTFE, the result of this evaluation is an unspecified constant. | ||
The compiler might be able to detect that UB occurred and raise an error or a warning, but this is not mandated, and absence of lints does not imply absence of 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 in this section we should explain the various mechanisms for detecting UB and their respective "detection rate" of UB.
- Pointer dereferences
- Dangling, null and allocation out of bounds pointers are reliably detected
- Out of bounds pointers that are within the allocation but outside any valid object are not detected
- Generally no guarantees are made for dereferencing arbitrary pointers with arbitrary types
- Uninitialized memory
- Values may be backed by an uninitialized value, any use of that value will cause an error
- Uninitialized memory in the final value of a constant is reliably detected, but intermediate computations may do arbitrary things
- Pointer comparisons
- Pointers cannot be compared, this is prevented statically and dynamically.
- The
guaranteed_eq
method on pointers allows fuzzy comparison of pointers, where a valid implementation is to always returnsfalse
and the code relying on the return value must still be valid iffalse
were returned in all cases. It is only to be used for performance reasons.
- Intrinsics
- Compiler intrinsics often have soundness rules for their arguments. e.g.
exact_div
forexact_div(x, y)
results in UB wherex % y != 0
or,y == 0
or,x == T::MIN && y == -1
but we reliably detect this.
- All intrinsics must detect all the UB that they can cause.
- Intrinsics that cannot have UB should be marked as safe
- Compiler intrinsics often have soundness rules for their arguments. e.g.
- transmute/union access/pointer dereferences
- It is entirely up to the user to ensure that the results of type punning derefs or actual transmutes result in valid values and does not depend on
repr(Rust)
types or other unspecified layouts.
- It is entirely up to the user to ensure that the results of type punning derefs or actual transmutes result in valid values and does not depend on
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.
Out of bounds pointers that are within the allocation but outside any valid object are not detected
I am not entirely sure what this even means, this is using C terminology ("object").
Values may be backed by an uninitialized value, any use of that value will cause an error
Here, "use" means strictly "actually performing an arithmetic or logic operation that this value as a input". A mere copy will not be detected (since we do not check validity).
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 in this section we should explain the various mechanisms for detecting UB and their respective "detection rate" of UB.
I have to say this list seems to be awfully geared around what happens to be the current implementation... in particular if we consider RFCs as applying to all Rust compilers, this seems unnecessarily specific.
I'd say something much shorter, more like:
- UB caused by inrinsics is detected.
- Dereferencing dangling pointers is detected. (But misalignment is not!)
- All the other UB listed here is not detected.
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 am not entirely sure what this even means, this is using C terminology ("object").
I tried to say we don't detect references that point to uninitialized memory. Like the extra capacity in a vector could be accessed by using set_len
wrongly and then indexing into the uninitialized parts.
Which kind of relates to your next point. Yes, I did mean actual operations, not just copying values.
I have to say this list seems to be awfully geared around what happens to be the current implementation... in particular if we consider RFCs as applying to all Rust compilers, this seems unnecessarily specific.
Do we want other Rust compilers to be more permissive? Your list is only missing uninit memory in final values and basic operations and pointer comparisons. While I get that pointer comparisons aren't really UB, they just aren't possible, at least doing math or so with uninitialized memory/undefined values should be UB. Or is there a difference between intrinsics and basic ops?
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.
Ah right we could also mention "uninit memory occurring as an input to an arithmetic, logical or control-flow operation".
While I get that pointer comparisons aren't really UB, they just aren't possible, at least doing math or so with uninitialized memory/undefined values should be UB. Or is there a difference between intrinsics and basic ops?
I'd like not to touch on unconst operations in this RFC; that is a more tricky subject that I planned to leave for the future.
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.
yea ,but shouldn't we do the const roadmap blogpost first?
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.
Should we? Not sure how the blog post would block even proposing the RFC.
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.
It doesn't block it, it's more... I was under the impression that the lang team kinda expects the next const thing to be given to them to be the roadmap that they'd sign off.
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.
This RFC was at least mentioned at the last meeting so I feel it is fine to be doing it in parallel. We'd still need this even after they signed off the road-map so I do not see a dependency either way here.
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.
That's true.
With the idea of running const evaluation on unoptimized MIR looking like a somewhat likely outcome here but there also being concerns about that impacting performance, I'm wondering (as somebody who has never written a compiler optimization): is there / could there be a small set of optimizations that are guaranteed to never exploit UB, which could be allowed to run for const eval? |
Yes, we do have optimizations like that. One example would be the const propagator, we'd have to vet all the other optimizations for whether their transformation is local enough to never have an effect on UB |
Indeed. Optimizations in general have to show that the optimized code has the same or fewer possible behaviors as the source code. However, some optimizations actually ensure that the new code has exactly the same possible behaviors as the source code (in particular, UB executions will always remain UB). Those we could still run. (They might degrade diagnostics, since the UB might now be caused elsewhere or so, but UB will at least still be detected.) |
Now I am confused, I was going to update the RFC to incorporate what we discussed.^^ |
oh... that hadn't happened yet, oops, well, open another PR? |
Will do, once I have some time. This affects most of the RFC after all.^^ |
Done: #60 |
At the const-eval lang team meeting a few months ago, we discussed how to handle UB during CTFE. This RFC reflects what I think was the consensus back then.
The "Reference-level explanation" is suspiciously short, but I really don't think there is terribly much to say here.^^
I figured we could briefly discuss this in the WG to make sure we all agree and add any bits that might be missing, and then I can make this an official RFC.
Cc @rust-lang/wg-const-eval