Repository navigation
Conversation
|
HIR ty lowering was modified cc @fmease |
rustbot
added
S-waiting-on-review
|
Thanks for the pull request, and welcome! The Rust Project has assigned @TaKO8Ki (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks. Please see the contribution instructions and our LLM policy for more information. Why was this reviewer chosen?The reviewer was selected based on:
|
rustbot
added
S-waiting-on-author
|
Converting to draft, while it passed unit tests locally I was able to cause an issue with the validator when running against code in the wild. I will reopen once resolved. Specifically, this case fails: I'm adding it to my tests now as a regression test, and I'm going to try to figure out why this is happening. Getting this error: My original design was based on the cmse.rs impl inside of |
d5d25f0 to
4610779
Compare
This comment has been minimized.
This comment has been minimized.
|
With a fairly minimal refactor I moved it into check (I'm still not confident on where to integrate it, so advice would be welcome) and the cycle error is resolved against my tests, however, the validation no longer runs against function pointers. I'm still a bit uphappy about that, but, for now, since I after running more tests on existing code (including the x86_64 crate) I found no regressions, I'm reopening. |
17c3dd0 to
123a52d
Compare
rustbot
added
S-waiting-on-review
rustbot
added
S-waiting-on-author
|
A quick reference for what shapes GCC/Clang/rustc allows / rejects (pre / post changes) ChartsAssume 64-bits except if noted otherwise. Rust types used: C types used: Frame parameter
Error code parameter
Whole-signature shape
32-bit variants
NotesMost significant things I note is:
Wrong Code (Array)Basically, error code resides at [rsp + 24], we load it as a pointer into rax and then dereference it twice for accesses into c[0] and c[2]. let err_code == 2, we then dereference 0x2, causing a fault. Wrong code (ZST Code)If we installed |
123a52d to
b4510f7
Compare
rustbot
added
S-waiting-on-review
|
I've implemented generics support and validated against most patterns. Not sure I got all patterns possible since the breadth of possible generic structures is quite high, but I tried to be thorough. |
This comment has been minimized.
This comment has been minimized.
b717173 to
7afa7f1
Compare
|
@rustbot reroll |
|
r? workingjubilee Not sure if this is correct to do, but this has been tossed around a bit and this directly addresses (or seeks to, at least) a TODO left in your name. |
|
|
|
@Siech0 It is correct as long as you are patient. A moment. |
|
So, it is not actually clear to me that zero-sized parameters must be rejected here as long as they are also zero-aligned. They would be skipped over when lowering to LLVMIR, of course, like we usually do with such arguments. (...usually.) #132835 is actually more along the lines of my concern. I reopened that issue. |
|
This would require fixing the codegen to actually ignore it though, of course. |
There was a problem hiding this comment.
Despite saying that, rejecting ZSTs here is probably easier than that, and we can always relax later if we care, even without stability concerns for this wildly unstable feature. So I can probably accept this if we fix some of this up a little, and we can address the zero-sized codegen thing in a separate PR if and only if you are interested, and then just relax that restriction. I don't want to quibble too much on the proper implementation of a feature I'm not sure will ever reach stable.
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
| // 1. 1-2 parameters (validated in AST layer) | ||
| // 2. Neither type may be unsized -> X86InterruptUnsized | ||
| // 3. Neither type may be zero-sized -> X86InterruptZeroSized | ||
| // 4. First parameter (frame) must be valid for any bit pattern -> X86InterruptInvalidFrame |
There was a problem hiding this comment.
I don't really understand the intent of this restriction.
If we consider this: https://docs.rs/x86_64/latest/src/x86_64/structures/idt.rs.html#1078-1095
If those "reserved" segments were replaced by padding, which would require adjusting a bunch of details about the struct but could be done at least partially, what would be incorrect about it? It's not like the CPU reads the ISR's frame back, right? We're not relying on writing to this, yeah?
There was a problem hiding this comment.
I may have been overly reading into (and probably misinterpreting!) the requirements of 'safe' rust. Currently the x86-interrupt abi doesn't explicitly require us to mark functions as unsafe, so I assumed data received in the signature must be valid according to those rules.
My thinking on the matter was that rust disallows types like bool to contain bit-signatures out of their range (i.e, bool may be 0 or 1, but 2 or 3 would be invalid), so following that logic, something like 5 bools separated by usize-1 padding should be invalid because the processor could hand you bit patterns outside of their valid range. Most Frame types I saw in the wild met this condition (usually just multiple usizes) and so I thought it was a fairly reasonable requirement at the time of writing.
In hindsight, my logic seems a bit flawed and overly terse but I still feel like there should be some level of requirement here but at the very least the array of bools case is certainly invalid? Do you think that simply removing the constraint is the reasonable way forward, or should we replace it with something less terse?
There was a problem hiding this comment.
So what you pointed out is actually a deeply interesting question, and is sort of at the heart of the problem of this ABI.
In Rust code, we only discharge an unsafe obligation on calling a function. How then do we describe the safety obligations of a function that must describe a correct signature in order for it to work, but which is called by an external force? Namely, the CPU's interrupt-handling firmware.
For extern things, in Edition 2024 we decided to make it so that every site which defines FFI functions and types like that is itself unsafe, e.g.
unsafe extern "C" {
fn blah();
}
fn main() {
unsafe { blah() };
}That is because merely linking the wrong thing... which is essentially what that declaration tries to do... can have negative effects if the signature is wrong. Your program can go to all sorts of exciting places if you misdeclare malloc, for example.
The unsafety of declaration does not inhere in the function we call, but which is why we allowed declaring functions safe at the same time. It still defaults to unsafe, because other languages don't have the same... ah, philosophy... that Rust does. Rust also is allowed to define a new extern "C" fn that is itself safe to call from Rust, and also can be called correctly by external C code:
unsafe extern "C" {
safe fn safe_blah();
}
extern "C" fn rust_blah() {}
fn main() {
// look ma, no unsafe!
rust_blah();
safe_blah();
}There was a problem hiding this comment.
...So the answer is "that actually is an entirely different category of safety obligation that we don't have the linguistic expressiveness to discharge properly if we decide to place it on the programmer".
There was a problem hiding this comment.
So I do kinda see where you're coming from: since no programmer can handle the obligation due to lacking language surface, let the compiler handle it! But... do we actually handle it? Is it impossible, after this PR, to not create a miscompiled function or one that reads de facto uninit memory by declaring, say, a 1000-byte stack frame?
There was a problem hiding this comment.
@Siech0 Also, sorry if my blather was a bit hard to read, I had lost a better-edited version and kinda rushed rewriting it.
Right, I had also pointed out that I explained that every unsafe fn does not itself make its contents unsafe code, which is what motivated the unsafe_op_in_unsafe_fn lint, e.g. Vec::set_len.
For note, it's not necessary for you to make a hard decision here or make this perfect for me to merge this PR!
I'm just wondering about this particular restriction because I am not sure it is either necessary or sufficient (i.e. it's not clear this over-restriction is enough) for safety here. If decls of these functions raise a hard problem here we can't easily resolve in the compiler, I can take that to lang and you don't have to get involved.
There was a problem hiding this comment.
Your write-up was informative, I think I mostly understand what you're getting at though I think some of it is a bit beyond my comprehension.
I've thought about it some and I think there a few ideas that might be an improvement:
- Simple size bounds ( frame <= 5 * usize ), alignment <= pointer size, drop requirement 4 in favor of this. This doesn't catch some UB but at least doesn't overcorrect for it.
- A compiler known
InterruptStackFrametype, like how we handle VaList for the variadics ABI. This kinda has a precedent, but it feels like a bit of a stretch to me. - We hand the frame via pointer like LLVM and GCC do, the deref safety obligation is moved to the body which is expressible. I think prior designs for the feature did this and they were changed and it is more in line with how the backend operates. I'm not sure if my memory is faulty or not. If there was a change, I don't know the rationale. Frankly, this seems like it quashes the majority of the can of worms because we can delegate correctness to the user. This change would probably be very much non-trivial and its probably not something I could accomplish at my current understanding level.
- We mark the ABI definition unsafe? I suppose this goes into what you were mentioning when talking with lang. (Though the full understanding of the implications is beyond me).
As an aside on the ZST parameters which I failed to fully cover:
- Frame: Allowing a ZST frame may be sensible, its essentially just us saying that we won't read / write with the parameter at all and we don't particularly care what its structure is.
- Error Code: Allowing a ZST error code may also be sensible, its essentially just us saying that we won't read / write with the parameter at all and we don't particularly care what its structure is. Currently this results in code-gen which aliases the function onto the frame-only ABI which results in the
Wrong code (ZST Code)error I marked above.
We could drop the ZST exclusion then if we think its sensible and just solve the code-gen and ICE issue to support them.
There was a problem hiding this comment.
re: #4, that is what I was waving in the direction of as not-communicable, yes, or rather not communicative?
Saying every extern "x86-interrupt" function is also unsafe does not immediately make it clear we are saying that the function is unsafe to declare incorrectly, because the "normal" meaning of unsafe fn is that you have to write
unsafe { unsafe_fn() };This notably says nothing about the actual contents of the unsafe fn or the declaration thereof, which are correctly handled by the Rust compiler for "normal" ABIs. Due to permissiveness in early Rust, you can declare an unsafe extern "C" fn that doesn't actually make much sense to C because it includes types that lack a representation in your platform's C ABI! ...but if anyone calls it, the ABI will be correct, even if it's more the, er, "Rust-enhanced C ABI". You can't cause the code to miscompile just because you gave such a "naughty" signature.
...well, the pile of bugs labeled A-ABI suggest "correctly" and "can't" is a bit of a stretch but it's the compiler's problem, not theirs.
There was a problem hiding this comment.
If, knowing this is technically "not the right way", you believe demanding these are unsafe fn will make people wary of misdeclaration concerns... yeah I'll believe that and I'm happy to ship it as a mitigation for the current feature.
There was a problem hiding this comment.
@Siech0 That aside, I think the first solution makes sense to me also and does seem simple. Exploring the 3rd approach and clearly identifying the reason to do it by-value should probably be required for stabilization of this feature, but that's not a concern we have to solve today.
Originally posted by @Siech0 in #162457 (comment) quick, you say. |
|
You drew attention to this case of these two signatures, @Siech0: void ISR f(struct frame *fr, unsigned char c[3])fn isr(_: Frame, _: [u8; 3])So, for the first parameter, in Rust we take the stack frame as a by-value argument, and in C it is a pointer to a stack frame. But as you might guess, the same thing happens in both languages and this is merely identifying data on your stack. The pointerness of it or not is irrelevant. But in the second parameter, you highlighted These are not equivalent signatures in C and Rust. The first one is genuinely actually a pointer, and is not even required to be of the length suggested by the signature because of course, C doesn't verify that. Meanwhile, in Rust, this is generally bundled into a by-value wad of data we pass along. That's why there is an FFI safety warning. So, for the Rust case, whatever C compilers do is irrelevant here because there's no equivalent. |
|
For the bad assembly cases, you didn't clearly identify which Rust signatures generated each? I'm a little confused. |
Sorry, I originally had that comment written out. I generated the data via a python script that would compile each and every case and record behaviors. Originally I had updated the table by hand to add my additional research on the miscompilations I found but I later added more cases and regenerated it (and forgot to update the links.) I'll spend some time this weekend trying to see if I can remember / recreate the exact cases that I did research on, but it has been a while. Off the top of my head, I believe |
Given the relative length that it could be.. perhaps? I tried to pretty comprehensively cover as many bases as I could while also not writing a full book on it. |
Good catch, I'll update the table, I wrote a lot of this undercaffinated and late into night. |
Unsized parameters or zero sized parameters provided to an function with the x86-interrupt abi could emit ICE. Such parameters are conceptually invalid. This commit implements parameter validation logic for the x86-iterrupt ABI to prevent these error classes and to improve user diagnostics.
bd36627 to
49a426f
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
It is completely okay to be wrong about something, especially something that does not particularly define the correctness of the PR itself. It just seemed important to actually reply to you calling attention to it, since, human conversation and all that. "All we need to do is make sure we keep talking". |
|
☔ The latest upstream changes (presumably #165876) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
View all comments
Unsized parameters or zero sized parameters provided to an function with the x86-interrupt abi could emit ICE. Such parameters are conceptually invalid. This commit implements parameter validation logic for the x86-iterrupt ABI to prevent these error classes and to improve user diagnostics.
Relevant unstable feature: #40180
Fixes #124806
Fixes #126418
(May also close this, a reviewer's input would be optimal: #63018. It depends on what the desired resolution is, because current rules don't bound the size requirements of frame though they could be trivially added. The bug itself seems to be resolved at this point.)
LLM assistance disclosure: An LLM was used in the development of this PR, specifically for the following tasks:
There exists no LLM generated code, comments, documentation, or tests in this PR.