Incorrect clashing_extern_decl warning in nightly

@jumbatm

PR #70946 introduces new clashing_extern_decl lint which causes incorrect warning even when two extern functions are ABI compatible:

#![allow(dead_code)]

#[repr(transparent)]
struct T(usize);

mod a {
    use super::T;
    extern "C" {
        fn test() -> T;
    }
}

mod b {
    extern "C" {
        fn test() -> usize;
    }
}

Some other example pairs that are ABI compatible that are incorrectly warned

fn test() -> usize and fn test()
fn test() -> usize and fn test() -> NonZeroUsize
fn test() -> usize and fn test() -> Option<NonZeroUsize>

This issue has been assigned to @jumbatm via this comment.

@jumbatm

cc @jumbatm @nagisa

@jumbatm

@jumbatm let me know if you are not able to look into this in near future.

@rustbot

Thanks for the report. I'll start looking into this tonight.

@rustbot claim

Good pickup on the #[repr(transparent)] case -- I'm working on fixing that now.

For the additional pairs you mention, it's not clear to me why they shouldn't warn:

fn test() -> usize and fn test()

This should be compatible because if the real signature for test is the first signature, and the second is the one supplied to Rust code, it's sound because it simply means the return type won't be read, right?

However, the other way around

// On the C side
void test() { /* ... */ }

and

// In the calling Rust code
extern "C" { fn test() -> usize; }

would let you read uninitialised memory, because the Rust code would look for a return value that the C side never set. It's not enough to assume that the first encountered declaration of the function is the correct one and only warn for a specific order of the clashing declaration, because at any point in the code, both versions of the function may be callable, in different modules-- we only know that at least one of the functions is incorrect. That being said, I definitely see how it's not clear in the current diagnostic why this is linted against, though.

fn test() -> usize and fn test() -> NonZeroUsize

I'm not sure about these two, because although this would not be unsound from a memory safety perspective, there's still a change in invariance between the two signatures. If the former is correct, then the latter's signature (and the corresponding invariant that the return value is non-zero) is incorrect and needs to be corrected anyway; if the latter's is correct, then there is an additional invariant that correcting the former adds -- a win-win situation for both cases.

fn test() -> usize and fn test() -> Option<NonZeroUsize>

This one's because the size of both return types is the same, right? This relies on the memory layout optimisation of None being 0, which I can't find any mention of being guaranteed (though, to be fair, I'd imagine is a pretty safe bet). It also needs#[allow(improper_ctypes)], which makes doing this seem a bit smelly. I'd be hesitant to not warn on this case unless None-is-0 is guaranteed (in which case, perhaps improper_ctypes would also need to except this case).

Ah... it seems like what these cases have in common is a differing view on whether clashing_extern_decl should be guarding solely against uninitialised memory reads (which implies to me that this lint would boil down to a simple size + alignment check), or whether the lint should be also guarding against "transmuting" memory reads (which makes these additional cases worth linting against). I'm of the opinion that this lint should also be guarding against transmuting reads, but I'd be keen to hear from any other perspectives.

Good pickup on the #[repr(transparent)] case -- I'm working on fixing that now.

For the additional pairs you mention, it's not clear to me why they shouldn't warn:

fn test() -> usize and fn test()

This should be compatible because if the real signature for test is the first signature, and the second is the one supplied to Rust code, it's sound because it simply means the return type won't be read, right?

However, the other way around
// On the C side
void test() { /* ... */ }
and
// In the calling Rust code
extern "C" { fn test() -> usize; }
would let you read uninitialised memory, because the Rust code would look for a return value that the C side never set. It's not enough to assume that the first encountered declaration of the function is the correct one and only warn for a specific order of the clashing declaration, because at any point in the code, both versions of the function may be callable, in different modules-- we only know that at least one of the functions is incorrect. That being said, I definitely see how it's not clear in the current diagnostic why this is linted against, though.

Well, we don't know about the actual declaration on the C-side. Even all FFI on the Rust side is compatible, there is no guarantee that it matches the C-side declaration. We are and always will depend on programmer to get that correct.

Because I believe there is an intention for this lint to become a hard error in the future, we will have to conservative when generating it, or we will need to different warnings for these two cases.

fn test() -> usize and fn test() -> NonZeroUsize

I'm not sure about these two, because although this would not be unsound from a memory safety perspective, there's still a change in invariance between the two signatures. If the former is correct, then the latter's signature (and the corresponding invariant that the return value is non-zero) is incorrect and needs to be corrected anyway; if the latter's is correct, then there is an additional invariant that correcting the former adds -- a win-win situation for both cases.

NonZeroUsize is a wrapper of usize with an attribute to restrict the range. Because C cannot express that, they'll need to be considered compatible on the Rust side. Similar things apply to NonNull pointers.

fn test() -> usize and fn test() -> Option

This one's because the size of both return types is the same, right? This relies on the memory layout optimisation of None being 0, which I can't find any mention of being guaranteed (though, to be fair, I'd imagine is a pretty safe bet). It also needs#[allow(improper_ctypes)], which makes doing this seem a bit smelly. I'd be hesitant to not warn on this case unless None-is-0 is guaranteed (in which case, perhaps improper_ctypes would also need to except this case).

NonZeroUsize is marked with #[rustc_nonnull_optimization_guaranteed], which guarantees that Option<NonZeroUsize> will always fit in a usize. Similar things apply to any non-nullable pointers.

Ah... it seems like what these cases have in common is a differing view on whether clashing_extern_decl should be guarding solely against uninitialised memory reads (which implies to me that this lint would boil down to a simple size + alignment check), or whether the lint should be also guarding against "transmuting" memory reads (which makes these additional cases worth linting against). I'm of the opinion that this lint should also be guarding against transmuting reads, but I'd be keen to hear from any other perspectives.

Just a note, size and alignment check is insufficient for capturing ABI compatibility issues. For example fn(f32) -> f32 is incompatible with fn(u32) -> u32 on most platforms, so just a size and alignment check is insufficient, and we would need something similar to what you implemented.

One idea, we might define something like

struct ExternDeclCompat {
    Compat,
    Incompat,
    Maybe(GuessedCFunctionType) // We unify and infer the actual C type that would make the two declaration compatible
}

and generate different warning for the incompatible and maybe compatible case.

Well, we don't know about the actual declaration on the C-side. Even all FFI on the Rust side is compatible, there is no guarantee that it matches the C-side declaration. We are and always will depend on programmer to get that correct.

Because I believe there is an intention for this lint to become a hard error in the future, we will have to conservative when generating it, or we will need to different warnings for these two cases.

The priority should be soundness here -- both cases need to be detected. Otherwise, we've only fixed a soundness hole half the time, which is to say not at all.

mod a {
    extern "C" { fn test() -> usize; }
}
mod b {
    extern "C" { fn test(); }
}

unsafe {
    b:: test();
    let s = a::test();
}

No matter what order mod a and mod b are declared in for this example, we know for certain that at least one of those declarations are incorrect, and it's precisely because we don't (and cannot) know the actual C side declaration that this should be warned (and later, errored) against, no matter what order things were declared in. Otherwise, if we just assume the first declaration is correct, we risk missing the case where a and b are declared in the order above, but then the real function doesn't return anything.

Because C cannot express that, they'll need to be considered compatible on the Rust side. Similar things apply to NonNull pointers.

This is true if we're comparing the C function to the Rust declaration. However, we're comparing the Rust side to the Rust side, without any knowledge of the C side:

mod c {
    extern "C" { fn foo() -> NonZeroUsize; }
}
mod d {
    extern "C" { fn foo() -> usize; }
}

In this scenario, the two signatures can't both be correct -- either the real function returns a usize that will never be zero, or it returns a usize which might be zero. Both can't be true at once, so at least one of these declarations must be incorrect.

NonZeroUsize is marked with #[rustc_nonnull_optimization_guaranteed], which guarantees that Option will always fit in a usize. Similar things apply to any non-nullable pointers.

Sorry, I wasn't clear -- I meant guaranteed as in future-proof (perhaps better wording would have been to ask whether this conversion has been promised to always work in the future). If not, client code shouldn't be relying on this conversion anyway as a future change to enum layout may inadvertently break the code, so we should not encourage this conversion by not linting against it. improper_ctypes doesn't make an exception for Option<NonZeroUsize>, which indicates to me that a the promise for this conversion to be future-proof hasn't been made, so we shouldn't except this case either. There's probably room for an RFC here to promise the compatibility of this conversion which, if accepted, would mean improper_ctypes would also need to except this case.

size and alignment check is insufficient for capturing ABI compatibility issues

Definitely agree with you here. Hence, the lint currently guards more than just uninitialised memory accesses, and guards against "transmuting" cases, as I mentioned above.

One idea, we might define something like
struct ExternDeclCompat {
    Compat,
    Incompat,
    Maybe(GuessedCFunctionType) // We unify and infer the actual C type that would make the two declaration compatible
}
and generate different warning for the incompatible and maybe compatible case.

Finding a common C type means the Rust compiler would have to understand (and adapt to updates in) C semantics. Generalising it out to the ABI, however (which the compiler needs to understand anyway) -- it's a good idea! It has been suggested before that I write a heuristic that determines compatible signatures. I'm not ruling this out, but I'd like to see the results of a crater-check run before I commit to something like this, because as the lint currently stands, it takes the very maintainable, and very safe bet that a function (whether external or not) can only have a single, correct signature (so inconsistencies in the declared signature on the Rust side are almost certainly a mistake). If it turns out that a large number of projects purposely rely on FFI functions being imported with different signatures to call them in certain ways (as I've already seen for some simd intrinsics), and those imports are largely sound, then this is definitely something I'm happy to implement.

For the usize and NonZeroUsize case, even though your logic that they are different is correct, in practice because NonZero* and NonNull types are sometimes somewhat difficult to use, so in practice people may just write usize or *mut xxx when even it returns non-zero. Anyway, you'll find that NonZeroUsize is actually just a wrapper over usize, just with a niche, so if you implemented the #[repr(transparent)] logic you'll find that is indeed allowed already.

#[rustc_nonnull_optimization_guaranteed] is a strong guarantee that this optimisation must be made and changing it is a breaking change. In fact, the attribute is introduced to make such a guarantee (#60300). ~~I think the fact that improper_ctypes does not detect it is a bug.~~ From my testing improper_ctypes is recognising Option<NonZeroUsize> just fine?

in practice people may just write usize or *mut xxx when even it returns non-zero

Ah, makes sense. The lint is only triggered if someone inconsistently declares the signature on the Rust side, though -- "fixing" the code causing this lint requires requires either marking the signature as a normal usize (which is the case you're describing, and is fine) or marking the other signature as a non-zero usize (which adds another invariant that both the callers and optimisations can rely on). This is a win-win situation, as I described earlier, so I'm against letting this case through.

#[rustc_nonnull_optimization_guaranteed] [ ... ] is introduced to make such a guarantee (#60300)

Thanks heaps for finding this. It's exactly the indication I needed to see (and failed to find). I'll add this exception to clashing_extern_decl.

From my testing improper_ctypes is recognising Option just fine?

Oh 🤦, you're right. I had originally written Option<usize> in my test case instead of Option<NonZeroUsize>, and had to #[allow(improper_ctypes)]. I'd then corrected it to Option<NonZeroUsize> and forgot to remove #[allow(improper_ctypes)].

Auto merge of rust-lang#73990 - jumbatm:clashing-extern-decl, r=nagisa

It seems that there is a case still not being fixed, when #[repr(transparent)] is used with non-zero types.

mod a {
    use std::num::NonZeroUsize;
    extern "C" {
        fn a() -> NonZeroUsize;
    }
}

mod b {
    #[repr(transparent)]
    struct X(NonZeroUsize);
    use std::num::NonZeroUsize;
    extern "C" {
        fn a() -> X;
    }
}

Thanks, I'll open another issue for this case.

Navigation Menu

Repository navigation

Incorrect clashing_extern_decl warning in nightly #73735

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Incorrect clashing_extern_decl warning in nightly #73735

Description

Activity

nbdd0121 commented on Jun 25, 2020

nagisa commented on Jun 25, 2020

jumbatm commented on Jun 25, 2020

jumbatm commented on Jun 27, 2020

nbdd0121 commented on Jun 27, 2020

nbdd0121 commented on Jun 27, 2020

jumbatm commented on Jun 27, 2020

nbdd0121 commented on Jun 27, 2020

jumbatm commented on Jun 28, 2020

nbdd0121 commented on Aug 20, 2020

jumbatm commented on Aug 20, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions