Skip to content
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

The improper_ctypes lint is very weak #36464

Closed
amluto opened this issue Sep 14, 2016 · 13 comments
Closed

The improper_ctypes lint is very weak #36464

amluto opened this issue Sep 14, 2016 · 13 comments
Labels
A-FFI Area: Foreign function interface (FFI) A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@amluto
Copy link

amluto commented Sep 14, 2016

This code compiles without warnings. I think that the improper_ctypes lint should catch both of the extern declarations.

#[allow(dead_code)]

#[repr(C)]
enum NotSoGood {
    A,
    B(i32),
}

extern "C" {
    fn foo(a: [u8; 16]);
    fn bar(a: NotSoGood);
}

#[no_mangle]
pub extern fn test() {
    unsafe {
        foo([1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16]);
        bar(NotSoGood::B(0));
    }
}

The array case (foo) is particularly nasty. On brief, insufficiently careful inspection, it looks like it matches:

void foo(uint8_t a[16]);

But it actually doesn't match that and instead seems to try to pass the array in packed form in xmm0 on x86_64. This is extra nasty because I think I've caught rust-bindgen generating bindings like this.

@sfackler sfackler added the A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. label Sep 14, 2016
@nagisa
Copy link
Member

nagisa commented Sep 14, 2016

I think that the improper_ctypes lint should catch both of the extern declarations.

The enum should be caught, but the extern declarations should not, because code like following is entirely valid:

// in rust
#[link_name="bar"]
pub extern fn stuff(a: NotSoGood) { ... }

// somewhere else, still in rust
extern { fn bar(a: NotSoGood); }

To explain, making functions be called using non-Rust calling convention does not invalidate passing rust data types back into Rust for functions implemented with non-Rust calling convention.

@retep998
Copy link
Member

void foo(uint8_t a[16]); in C/C++ does not actually mean what it looks like. It actually means void foo(uint8_t* a);. So the fault lies with C/C++ being confusing in this case.

@amluto
Copy link
Author

amluto commented Sep 14, 2016

@nagisa: I agree that your code is valid, but you typed extern and not extern "C". There's a C ABI, and that ABI, as a practical matter, does not include passing arrays by value, since you can't pass arrays by value in C.

Oddly, the actual x86_64 psABI does seem to think that arrays can be passed:

The classification of aggregate (structures and arrays) and union types works as follows:  ...

and I haven't found an example of Rust deviating from the psABI document, but I still suspect that almost (or maybe even exactly) 100% of cases where someone has actually attempted to pass an array into an extern "C" function are incorrect.

@nagisa
Copy link
Member

nagisa commented Sep 14, 2016

externextern "C"

@amluto
Copy link
Author

amluto commented Sep 14, 2016 via email

@RalfJung
Copy link
Member

RalfJung commented Jan 22, 2019

@nagisa (I know I'm a bit late, what can you do) In your example, rustc actually will warn on the extern { fn } side of things. How is the trade-off different on the extern fn side?

@nagisa
Copy link
Member

nagisa commented Jan 22, 2019

The #[repr(C)] enum now has a formal definition, so this issue looks like it can be closed.

In your example, rustc actually will warn on the extern { fn } side of things.

Will it? All types in the example are repr(C), so there is nothing to warn about on either of the function definition or declaration.

@nagisa nagisa closed this as completed Jan 22, 2019
@nagisa
Copy link
Member

nagisa commented Jan 22, 2019

Ah I guess there is still the case with sized arrays.

@nagisa nagisa reopened this Jan 22, 2019
@jonas-schievink jonas-schievink added A-FFI Area: Foreign function interface (FFI) T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 29, 2019
@est31
Copy link
Member

est31 commented Oct 28, 2020

Ah I guess there is still the case with sized arrays.

Did #66305 fix this case? Can it be closed now?

@mversic
Copy link

mversic commented Aug 16, 2022

The following code doesn't trigger improper_ctypes lint. Should it not?

#[repr(transparent)]
pub struct Array([u64; 2]);
#[no_mangle]
pub unsafe extern "C" fn function(arg: Array) -> Array {
    arg
}

Referred to in this issue

@mversic
Copy link

mversic commented Aug 16, 2022

I guess the question above is: do arrays and structs follow the same calling convention? If no, then improper_ctypes lint should be improved to warn in the previous example

@Gankra
Copy link
Contributor

Gankra commented Aug 16, 2022

I believe this the ctypes lint isn't working correctly on that transparent array input, yes.

@workingjubilee
Copy link
Member

I have opened #116959 to cover the second case mentioned, because it is separate-ish. Closing.

@fmease fmease added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. and removed A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. labels Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-FFI Area: Foreign function interface (FFI) A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests