Skip to content

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Sep 3, 2018

CC #7.

@tbu-
Copy link
Contributor Author

tbu- commented Sep 3, 2018

How do I test this?

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@tbu-
Copy link
Contributor Author

tbu- commented Sep 3, 2018

(On Linux with glibc, it seems to work.)

@alexcrichton
Copy link
Member

Hm unfortunately I don't think libc-test is capable of testing static imports right now. I'd prefer to fix that before landing this, as having this in src/lib.rs seems unlikely to work for all platforms. Would you be interested in sending a PR to ctest to add support for a static?

@tbu-
Copy link
Contributor Author

tbu- commented Sep 4, 2018

Done: https://github.com/alexcrichton/ctest/issues/36.

The new tests pass on Linux with glibc.

@alexcrichton
Copy link
Member

Want to update the dependency here and see if it passes CI?

@tbu-
Copy link
Contributor Author

tbu- commented Sep 4, 2018

Summarizing the failures:
macOS: std* not defined, not investigated further.
musl: std* defined as FILE *const instead of glibc's FILE *.
Android: Unrelated failure, due to the now-existing static checking: __progname not found in the included headers, declared in src/unix/notbsd/android/mod.rs:1516.
NetBSD: std* is just a macro for something else (permitted by the C standard, std* just needs to be a macro which resolves to an expression for the relevant stream): NetBSD source: include/stdio.h:215-217, include/stdio.h:149.
FreeBSD: std* is a macro referencing a static with a different name (__std*p): FreeBSD source: include/stdio.h:240-242, include/stdio.h:169-171.

@tbu-
Copy link
Contributor Author

tbu- commented Sep 4, 2018

Since it's only required to be a value by the C standard, maybe declaring it as a static is not the right way. Would you consider adding it as functions fn stdin() -> *mut FILE, similar to how the stdin identifier might only represent a value in C?

It seems we don't have bindings for errno so far, a similar thing could be done for that, I think.

@alexcrichton
Copy link
Member

The libc crate only defines what's already in libc, so we can use things like #[link_name] to rename it, but we can't insert our own shims.

@tbu-
Copy link
Contributor Author

tbu- commented Sep 5, 2018

stdin is part of libc. Rust doesn't have a 1-1 replacement for macros.

Maybe this could be part of a module, macros, so it doesn't clash with a potentially existing symbol stdin?

(It would be a function like

mod macros {
    fn stdin_value() -> *mut FILE {
        stdin
    }
}

or on NetBSD

mod macros {
    fn stdin_value() -> *mut FILE {
        &__sF[0]
    }
}

doing the same as the macro.)

@alexcrichton
Copy link
Member

I don't really have a preference on how this is bound, but it cannot be done through in-crate shims

@tbu- tbu- closed this Sep 5, 2018
@tbu-
Copy link
Contributor Author

tbu- commented Sep 5, 2018

So, if there were any change to this policy of not including macros like stdin, it would have to go through an RFC?

@alexcrichton
Copy link
Member

Yes I believe we'd need an RFC to change the policy of this crate

@thomcc
Copy link
Member

thomcc commented Jul 7, 2021

It seems like NetBSD is the only place where this needs a shim. The other places where macros are used (freebsd/darwin), it can be done with #[link_name].

Given that the libc crate already doesn't expose everything availiable on every OS, it seems plausibly okay to leave that out for NetBSD (and I guess any other OS like this).

Would a patch that did that have a shot of being accepted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants