lint ImproperCTypes: overhaul (take 2 of "better handling of indirections") #134697
Conversation
rustbot
added
S-waiting-on-review
|
( |
|
ah, and before I forget: a small part of the new test file is commented out because it hits ICE #134587, but there should be more than decent coverage anyway |
|
unfortunately the lint needs to be gutted and rewritten. |
|
Also while I was possibly having a mild case of get-there-itis and thus mostly tried to just make sure things were coherent, I would prefer all new code for the lint be in |
workingjubilee
added
S-waiting-on-author
|
The version cut happened so there will be less time pressure now. |
|
I have a first version for you probably have things to say about its architecture, even if the whole thing still have a bunch of TODO comments
If you want to take a look in this state, should I just commit it here? (possibly put the PR in draft mode while I'm at it?) |
|
☔ The latest upstream changes (presumably #135525) made this pull request unmergeable. Please resolve the merge conflicts. |
niacdoial
force-pushed
the
linting-ptrdyn-ffi
branch
from
5764b36 to
6c19cff
Compare
|
aaaand I think I have something that's "first draft" material! (should I put this pull in the "draft" state?) here's a list of some of my remaining questions and concerns:
|
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #135921) made this pull request unmergeable. Please resolve the merge conflicts. |
niacdoial
force-pushed
the
linting-ptrdyn-ffi
branch
from
6c19cff to
13720e7
Compare
This comment has been minimized.
This comment has been minimized.
|
hmm. |
workingjubilee
added
S-waiting-on-review
Yes, it's a good marker for "I don't want this merged yet, even if it looks done". |
It probably is.
Yes, but in particular, not to just repartition them between: I think breaking them into as many conceptually-smaller lints as possible is good, as long as each one is a distinct idea (no splitting just for the sake of splitting!).
I'm not sure what you mean?
We must allow Rust code to declare a pointer in a C signature to be
Yes, probably. |
| // but for some reason one can just go and write function *pointers* like that: | ||
| // `type Foo = extern "C" fn(::std::ffi::CStr);` |
There was a problem hiding this comment.
- Because unsized function parameters are something we may want to support.
- The code may not be well-formed: as you may have noticed at some point, you get warnings even if you get errors (usually), and this is because we lint even on "bad" code. This is because rustc didn't use to, once upon a time, and it was a bad debugging experience.
workingjubilee
added
S-waiting-on-author
|
alright, sorry for taking a while! I'm currently planning what changes I'll do in terms of splitting the lint(s)
well, this is more or less answered in what I said before that, but my question was about how to deal with "switching" from checking arguments for, say, a function definition, to checking the arguments of a FnPtr argument?
I... maybe? I can't for the life of me find the link to that again but I think I saw a discussion about that and type covariance/contravariance, Though you'll definitely know more than me on all the moving parts. As for the rest of your advice, I already took all this in! |
Mainly, we realise that the non-null assumption on a Box<_> argument does not depend on what side of the FFI boundary the function is on. And anyway, this is not the way to deal with this assumption being maybe violated.
no visible changes to rust users, just making the inner architecture of the ImproperCTypes lints more sensible, with a clean separation between the struct (now singular) that interacts with the linting system and the struct (now singular) that visits the types to check FFI-safety
No changes should be visible by rustc users This is just some architecture changes to the type checking to facilitate FFI-safety decisions that depend on how the type is used (the change here is not complete, there are still bits of "legacy" state passing for this, but since this is a retconned commit, I can tell you those bits will disappear before the end of the commit chain) (there is at least one bit where the decision making code is weird, but that this is because we do not want to change the lint's behaviour this early in the chain)
Another interal change that shouldn't impact rustc users. The goal is to break apart the gigantic visit_type function into more managable and easily-editable bits that focus on specific parts of FFI safety.
Another change that only impacts rustc developers: Added the necessary changes so that lints are able to specify in detail "A in unsafe because of its B field, which in turn is unsafe because of C, etc", and possibly specify multiple help messages (multiple ways to reach FFI-safety)
Simple change to stop irregular recursive types from causing infinitely-deep recursion in type checking.
First retconned commit in this change to impact rustc users: The improper_ctypes_definitions has been split into improper_c_fn_definitions and improper_c_callbacks, with the former lint name being turned into a lint group, so that users aren't forced to immediately change their code. Deprecating this old name will be left as an exercise to whichever team is in charge of breaking changes. Another lint group has been created to deal with all `improper_c*` lints at once.
another user-visible change: change the messaging and help around CStr/CString lints
- Uniformise how indirections (references, Boxes, raw pointers) are handled. (more specific indirection types with specific guarantees are not handled yet) - Indirections that are compiled to a "thick pointer" (indirections to slices, dyn objects, *not* foreign !Sized types) have better messaging around them. - Now, the pointee of a FFI-safe indirection is always considered safe. This might be a regression, if we consider that an extern function's API should describe how the function can be used by the non-defining side of the FFI boundary. However, enforcing this everywhere would force the user to perform an unreasonable amount of typecasts to/from opaque pointers. There is something better to do here, but it will be left to another PR.
Notably, those FnPtrs are treated as "the item impacted by the error", instead of the functions/structs making use of them.
A major change to the content of linting messages, but not where they occur. Now, the "uses type `_`" part of the message mentions the type directly visible where the error occurs, and the nested note/help messages trace the link to the actual source of the FFI-unsafety
A change in how type checking goes through structs/enums/unions, mostly to be able to yield multiple lints if multiple fields are unsafe
correctly handle !Sized arrays at the tail-end of structs and a cosmetic change to the array/slice-related lints,
Add some logic to the type checking to refuse uninhabited types as arguments, and treat uninhabited variants of an enum as FFI-safe if at least one variant is inhabited.
Properly treat the fact that a pattern can create assumptions that are used by Option-like enums to be smaller, making those enums FFI-safe without `[repr(C)]`.
Put the handling of opaque aliases in the actual `visit___` methods instead of awkwardly pre-checking for them
Add new areas that are checked by ImproperCTypes lints: Function declarations(*) and definitions in traits and impls *) from the perspective of an FFI boundary, those are actually definitions
Added the missing case for FFI-exposed pieces of code: static variables with the `no_mangle` or `export_name` annotations. This adds a new lint, which is managed by the rest of the ImproperCTypes architecture.
`[repr(C,packed)]` structs shouldn't be considered FFI-safe
- ensure proper coverage of as many edge cases in the type checking as possible - test for an issue that was fixed by this commit chain - understand where `[allow(improper_c*)]` needs to be to take effect (current behaviour not ideal, one should be able to flag a struct definitition as safe anyway)
smooth things out to avoid conflicts with rust-lang/compiler-builtins#1006 which has at time of writing not made it into rust-lang/rust's main branch
Rustc is trying to shift away from test names that are just issue numbers, so we add this as part of its effort, since this commit chain is moving and rewriting these files anyway. The directory containing all tests is also renamed.
niacdoial
force-pushed
the
linting-ptrdyn-ffi
branch
from
22b00d0 to
59de843
Compare
There was a problem hiding this comment.
Oh hey, jj is the perfect tool for big PRs like this :)
Maybe worth asking on the discord? https://discord.gg/wgp7xptM
| } | ||
| } | ||
|
|
||
| declare_lint! { |
There was a problem hiding this comment.
Commit "ImproperCTypes: move code and tests into proper directories": Nit, looks like the lint declarations got moved to the bottom of the file under everything for support? Usually lints are the other way around so you see the list as soon as you open the file (same usually for modules where there's one important/core type. I know this is a bit reversed from C)
There was a problem hiding this comment.
ok!
Should I also move, say, the ImproperCTypesLint struct/impl before the visitor class and other helper classes?
Or in general put the more important stuff (and/or the stuff that's connected to the outside code) near the start of the file?
There was a problem hiding this comment.
In general, yeah I think helper structs/functions tend to go toward the end of the file in Rust so you get the bigger pictures first as you read from the top down. But don't bother changing any other than declare_lint!, no need to cause yourself a bunch of conflicts (and even moving declare_lint is 🤷♂️ if it's not easy to do).
Languages that require declaration before use tend to do this the other way around, but they also don't really have the choice (unless you forward declare absolutely everything). Took me a little while to get used to the reverse :)
| // the values that can be set | ||
| // FIXME(const): ideally this would be in the impl block | ||
| // unfortunately, "cannot call non-const operator in constants" (bitwise or?). | ||
| const None = 0b000000; | ||
| const StaticTy = 0b000001; //Self::STATIC | ||
| const ArgumentTyInDefinition = 0b001010; //Self::FUNC | Self::DEFINED | ||
| const ReturnTyInDefinition = 0b001110; //Self::FUNC | Self::FN_RETURN | Self::DEFINED | ||
| const ArgumentTyInDeclaration = 0b000010; //Self::FUNC | ||
| const ReturnTyInDeclaration = 0b000110; //Self::FUNC | Self::FN_RETURN; | ||
| const ArgumentTyInFnPtr = 0b010010; //Self::FUNC | Self::THEORETICAL; | ||
| const ReturnTyInFnPtr = 0b010110; //Self::FUNC | Self::THEORETICAL | Self::FN_RETURN |
There was a problem hiding this comment.
Commit "ImproperCTypes: redo state tracking": It's a bit clunky but you can do:
impl Foo {
const ARG_TY_IN_DEF: Self = Self::from_bits(Self::FUNC.bits() | Self::DEFINED.bits()).unwrap();
}https://docs.rs/bitflags/latest/bitflags/#multi-bit-flags does say that as-is should be okay though, so that bit is up to you (I'd have a mild preference to use .bits()/from_bits() to reduce the chance for mistakes as this gets updated). But some of these are only used in one location so it would be fine to inline them directly there.
In either case please change consts to the conventional upper snake case :)
There was a problem hiding this comment.
Also for None, VisitorState::empty() can be used (it's const)
There was a problem hiding this comment.
...ok I should have spent more time in that crate's docs. thanks!
There was a problem hiding this comment.
To be fair, I think the docs could be a bit better - took me some trial and error to figure that out 😆
| #[cfg(debug_assertions)] | ||
| if ret { | ||
| assert!(self.is_in_function()); | ||
| } |
There was a problem hiding this comment.
if ret {
debug_assert!(self.is_in_function());
}Slightly simpler
There was a problem hiding this comment.
...fair, the condition itself is probably optimised out when debug_assertions are off.
There was a problem hiding this comment.
If you're curious, dead blocks are usually pruned at any nonzero optimization level by the first CSE (common sub-expression) pass https://rust.godbolt.org/z/GPbY9zEbj
| if ret { | ||
| assert!((self & Self::STATIC) == Self::None); | ||
| } | ||
| ret |
There was a problem hiding this comment.
Did this mean to be debug_assert! as well? (Probably doesn't really matter)
There was a problem hiding this comment.
my reasoning was:
the main difference is that there is a value assigned that is neither STATIC nor FUNCTION (None, which will be assigned in a later commit)
so, the debug_assert! only needs to ensure that non of the VisitorState values are broken, while the assert! acts as a bug! to guard that None from getting in the wrong piece of logic.
There was a problem hiding this comment.
In that case, it's somewhat preferred to use bug! directly since they show nicer ICE messages (a bit more about that at
rust/compiler/rustc_middle/src/macros.rs
Lines 1 to 13 in fd75a9c
if is_ret && self.contains(Self::STATIC) {
bug!(...)
}There was a problem hiding this comment.
turns out I misremembered something, so er...
both get debug_assert now.
| let ret = (self & Self::FUNC) != Self::None; | ||
| if ret { | ||
| assert!((self & Self::STATIC) == Self::None); |
There was a problem hiding this comment.
Bitflags gives you some convenience methods to avoid the bitwise ops: these could be self.contains(Self::FOO) and !self.contains(Self::STATIC).
Same for a handful of places below.
| if !matches!(pat_ty.kind(), ty::Int(..) | ty::Uint(..) | ty::Float(..) | ty::Char) { | ||
| bug!( | ||
| "this lint was written when pattern types could only be integers constrained to ranges" | ||
| ) | ||
| } |
There was a problem hiding this comment.
I think this could be eliminated or replaced with a comment since visit_numeric has the same bug!
There was a problem hiding this comment.
fair!
one, sort of unrelated question. Should I fix-and-push what you point out as soon as possible, or should I wait you to tell me the review is complete?
There was a problem hiding this comment.
I'd suggest amending the history as you fix things (thanks jj for making this a bit easier) and push whenever you're at a reasonable chunk point. That gives me a chance to mark some of these discussion items as resolved and check off more commits in my list at the top post before I'm 100% through everything. Also less review churn since the later commits will be closer to their final form.
| help: Some(fluent::lint_improper_ctypes_array_help), | ||
| } | ||
| } else { | ||
| FfiResult::FfiSafe | ||
| } | ||
| } |
There was a problem hiding this comment.
Commit "re-separate linting and checking": This pattern doesn't seem quite right; the type not being an array of course doesn't mean the type is FFI-safe. Could this and similar functions instead return an Option<FfiResult>? And only return Some(FfiResult::FfiSafe) when it is known to be safe.
Maybe good to document this with an alias:
/// The result when a type has been checked but perhaps not completely. `None` indicates that
/// FFI safety/unsafety has not yet been determined, `Some(res)` indicates that the safety/unsafety
/// in the `FfiResult` is final.
type PartialFfiResult<'tcx> = Option<FfiResult<'tcx>>;An alternative is to add an FfiResult::Unknown variant and return that, but I don't think that's worth propagating.
There was a problem hiding this comment.
[rolling this comment back, I misremembered things]
| } | ||
|
|
||
| fn check_for_opaque_ty(&mut self, sp: Span, ty: Ty<'tcx>) -> bool { | ||
| fn visit_for_opaque_ty(&mut self, ty: Ty<'tcx>) -> FfiResult<'tcx> { |
There was a problem hiding this comment.
Commit "re-separate linting and checking": Similar here, I think this could return None rather than FfiSafe
| fn check_for_type( | ||
| &mut self, | ||
| sp: Span, | ||
| ty: Ty<'tcx>, | ||
| is_static: bool, | ||
| is_return_type: bool, | ||
| ) { | ||
| if self.check_for_opaque_ty(sp, ty) { | ||
| // We've already emitted an error due to an opaque type. | ||
| return; | ||
| ) -> FfiResult<'tcx> { |
There was a problem hiding this comment.
Commit "re-separate linting and checking": could you add a doc comment for this function? Also if it's only checking a single type, check_type is probably a better name.
I think this is where it makes sense to return FfiResult<'tcx> (as you have), where something is known to either be FFI-safe or unsafe.
| /// For a function that doesn't need to be "ffi-safe", look for fn-ptr argument/return types | ||
| /// that need to be checked for ffi-safety. |
There was a problem hiding this comment.
For a function that doesn't need to be "ffi-safe" -> For a function that may not need to be "ffi-safe" since this is used for all functions right?
There was a problem hiding this comment.
not yet, but it will in a later commit in this chain.
I'm using a different phrasing:
Regardless of a function's need to be "ffi-safe",
| if let hir::FnRetTy::Return(ret_hir) = decl.output { | ||
| for (fn_ptr_ty, span) in self.find_fn_ptr_ty_with_external_abi(ret_hir, sig.output()) { | ||
| self.check_type_for_ffi_and_report_errors(span, fn_ptr_ty, false, true); | ||
| fn check_arg_for_power_alignment(&mut self, ty: Ty<'tcx>) -> bool { |
There was a problem hiding this comment.
Could you add a doc comment?
| /// Find any fn-ptr types with external ABIs in `ty`. | ||
| /// | ||
| /// For example, `Option<extern "C" fn()>` returns `extern "C" fn()` | ||
| fn find_fn_ptr_ty_with_external_abi( | ||
| &self, | ||
| /// For example, `Option<extern "C" fn()>` returns `extern "C" fn()`. | ||
| fn check_type_for_external_abi_fnptr( |
There was a problem hiding this comment.
Is this doc comment accurate? It looks like there is no return type here, and that it checks/emits them rather than returning the inner type
| all_types.for_each(|(fn_ptr_ty, span)| { | ||
| let mut visitor = ImproperCTypesVisitor { cx, mode: fn_mode }; | ||
| // TODO: make a check_for_fnptr | ||
| let ffi_res = visitor.check_for_type(fn_ptr_ty, is_static, is_return); | ||
|
|
||
| self.process_ffi_result(cx, span, ffi_res, fn_mode) | ||
| }); |
There was a problem hiding this comment.
Nit: flip to for (fn_ptr_ty, span) in all_types rather than for_each (makes it easier to spot loops)
rustbot
added
S-waiting-on-author
|
Once you address the comments here, could you add a bookmark/branch at the fourth commit "redo state tracking" and put that into a separate PR? I think up to that point should be fine to merge now, which cuts down on the rest here. Request me for review once you do so. Since you're using jj and it's easy to have multiple bookmarks, I think "split type visiting into subfunctions" and "add recursion limit" would also be good revs to put into separate PRs to merge before the rest (just mark them as draft) |
This PR started as a try to re-add the changes of #131669 (reverted in #134064 after one (1) nightly)
It has since evolved in an overhaul of the ImproperCTypes lints, splitting the two lints into four:
extern "ABI"blocksextern "ABI"functions#[no_mangle]/1[export_name=_]static variablesextern "ABI"callbacks (ty::FnPtrarguments/return types/ADT fields)It also allows for "detailed" lint messages, with successive notes that amount to "this is unsafe because of that, which is unsafe because of that, which is unsafe for this reason", and possible help messages at every step.
Hopefully the overall architecture of the lint is something less special-case-y, with better-separated abstractions.
the comment containing the most recent summary of the decisions and considerations is #134697 (comment)
oh, also, since this was already fixing it for the wrong reasons, I decided to add something that fixes it for the right ones:
Fixes #130310
Fixes #132699 (though not necessarily for the right reasons?)
(leaving this in because it was in the original version of this description:)
r? workingjubilee (because you reviewed the first attempt)
Review status:
_/-change: R-b TG