Skip to content

Zero-sized functions pointers no longer variadic-compatible #32201

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
alexcrichton opened this issue Mar 11, 2016 · 8 comments
Closed

Zero-sized functions pointers no longer variadic-compatible #32201

alexcrichton opened this issue Mar 11, 2016 · 8 comments
Assignees
Labels
P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Seems like a crazy edge case, but this code works on stable but is currently broken on nightly.

#![crate_type = "lib"]

extern {
    fn foo(a: i32, ...);
}

extern fn bar() {}

pub fn bad() {
    unsafe {
        foo(0, bar);
    }
}

On stable Rust, this produces the IR:

define void @_ZN3bad20h6e7ac30c4401351fnaaE() unnamed_addr #2 {
entry-block:
  tail call void (i32, ...) @foo(i32 3, void ()* nonnull @_ZN3bar20h7198ce9121a8c42ckaaE)
  ret void
}

whereas on nightly it looks like:

define void @_ZN3bad20had7770d3eb571d55naaE() unnamed_addr #1 {
entry-block:
  tail call void (i32, ...) @foo(i32 3, {} undef)
  ret void
}

Which is clearly bad! I noticed there's a warning for transmuting function types to function pointers (as now it's 0-sized to pointer-sized), but perhaps the same warning could be appplied here? Or better could the coercion be automatically applied?

For reference this was discovered in curl-rust which defines variadic functions.

cc @eddyb, @nikomatsakis

@alexcrichton alexcrichton added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Mar 11, 2016
@alexcrichton
Copy link
Member Author

triage: I-nominated

@alexcrichton alexcrichton added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 11, 2016
@eddyb
Copy link
Member

eddyb commented Mar 11, 2016

I... can't even:

extern {
    fn printf(_: *const u8, ...);
}

pub fn main() {
    unsafe {
        printf("'%s' has length %d\0".as_ptr(), "foobar\0");
    }
}
'foobar' has length 7

Works on playpen, shows no warnings...

@nikomatsakis
Copy link
Contributor

/me sighs. Hate variadics. I imagine though that we could do some sort of special case for ... arguments.

@eddyb
Copy link
Member

eddyb commented Mar 12, 2016

I think we should force a small set of types in rustc_typeck::check::check_argument_types for variadic arguments, and TyFnDef can be deterministically coerced to a pointer (like we do for casts).

@nikomatsakis
Copy link
Contributor

triage: P-high

@rust-highfive rust-highfive added P-high High priority and removed I-nominated labels Mar 17, 2016
@ben0x539
Copy link
Contributor

Shouldn't it be sufficient to restrict the arguments to variadic C functions to repr(C) things?

@eddyb
Copy link
Member

eddyb commented Apr 8, 2016

Turns out we do have some checks related to promotions, instead of actually performing them.
There were in check_argument_types all along, right under my nose.
Passing bool, i8, u8, i16, u16 or f32 is an error, and I can just add another case for functions.

Manishearth added a commit to Manishearth/rust that referenced this issue Apr 8, 2016
 Fixes rust-lang#32201 by adding fn types to the variadic blacklist which currently includes `bool`, `i8`, `u8`, `i16`, `u16` and `f32`.
@ranma42
Copy link
Contributor

ranma42 commented Apr 8, 2016

@eddyb Should we also blacklist fat pointers?

Manishearth added a commit to Manishearth/rust that referenced this issue Apr 10, 2016
Blacklist fn item types from being used with variadic functions.

Fixes rust-lang#32201 by adding fn types to the variadic blacklist which currently includes `bool`, `i8`, `u8`, `i16`, `u16` and `f32`.
bors added a commit that referenced this issue Apr 10, 2016
Blacklist fn item types from being used with variadic functions.

Fixes #32201 by adding fn types to the variadic blacklist which currently includes `bool`, `i8`, `u8`, `i16`, `u16` and `f32`.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants