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

Add support for making functions `const` #1536

Merged
merged 6 commits into from Nov 18, 2019

Conversation

@Aaron1011
Copy link
Contributor

Aaron1011 commented Sep 30, 2019

PR rust-lang/rust#64906 adds the ability to write const extern fn and const unsafe extern fn, which will allow manys functions in libc to become const.

This is particuarly useful for functions which correspond to C macros (e.g. CMSG_SPACE). In C, these macros are constant expressions, allowing them to be used when declaring arrays. However, since the corresponding libc functions are not const, writing equivalent Rust code is impossible. Users must either perform an unecessary heap allocation, or pull in bindgen to evaluate the macro for specific values (e.g. CMSG_SPACE(1)).

However, the syntax const extern fn is not currently parsed by rust. To allow libc to use this without breaking backwards compatibility (i.e. bumping the minimum Rust version), I've taken the following approach:

  1. A new off-by-default feature extern-const-fn is added to libc.
  2. The internal f! macro has two versions, selected at compile-time by a cfg_if. When extern-const-fn is enabled, the declared f! macro passes through the const keyword from the macro user to the final definition (pub const unsafe extern fn foo. When extern-const-fn is disabled, the const keyword passed by the macro user is discarded, resulting in a plain pub extern const fn being declared.

Unfortunately, I couldn't manage to get macro_rules to accept a normal const token in the proper place (after pub). I had to resort to placing it in curly brackets:

pub {const} fn foo(val: u8) -> i8 {
}

The f! macro then translates this to a function definition with const in the proper position.

I'd appreciate it if someone who's more familiar with macro_rules! could see if I missed a way to get the desired syntax.

@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Sep 30, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (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.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Sep 30, 2019

We probably want to test enabling the extern-const-fn feature on CI, but I'm not sure how to best add that to the existing CI setup.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Oct 7, 2019

@gnzlbg: Now that rust-lang/rust#64906 has been merged, this PR is no longer blocked.

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Oct 10, 2019

Why are these functions extern "C" in the first place ?

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Oct 10, 2019

@gnzlbg: I'm not sure. However, changing it would be a breaking change, and I don't see any harm in them being extern "C"

src/macros.rs Show resolved Hide resolved
Copy link
Collaborator

gnzlbg left a comment

The only thing missing is documenting the feature in the README, but otherwise this LGTM.

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Oct 10, 2019

Also @Aaron1011 do you know if libstd uses any of these functions? Right now, libstd won't enable this nightly feature, but I'm not sure if its worth it to do so.

@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/const-fn branch from d9161b5 to a1d1407 Oct 11, 2019
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Oct 11, 2019

@gnzlbg: Updated. Do we want any kind of CI test for this?

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Oct 15, 2019

@Aaron1011 yes.

One way would be to add a test file to libc/tests that's only included if the feature is defined, e.g., by using a #![cfg(feature = "...")] at the top of the file, and then in that file have some tests that for this target tries to call the API and store it in a const variable. After that, you need to modify ci/run.sh to run the tests of libc with the new --feature when the toolchain is a nightly toolchain. Let me know if you need help with that.

Add a new feature to enable this, since `const extern fn`
support is unstable
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Oct 28, 2019

@gnzlbg: I've been having a hard time getting this to work locally. Can you show me an example of what you want this to look like?

src/lib.rs Outdated Show resolved Hide resolved
src/macros.rs Outdated Show resolved Hide resolved
@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Oct 28, 2019

@Aaron1011 what I had in mind is a file in libc/tests/const_fn.rs that looks like this:

#![cfg(libc_const_extern_fn)] // If this does not hold, the file is empty

#[cfg(target_os = "linux")]
const _FOO: ... = CMSG_SPACE(...); 
//^ if CMSG_SPACE is not const, this will fail to compile

and just calls the const fn and stores their result in a const to trigger a compilation failure if they aren't const fn.


Note, the two suggestions above to use const_extern_fn instead of feature = "..." require defining libc_const_extern_fn in the build.rs if the toolchain is a nightly toolchain only. That is, the build.rs should, if the cargo feature is defined, check if the toolchain is a nightly toolchain, and only then define --cfg libc_const_extern_fn (check the build.rs since we already do this for other features). If the toolchain is not a nightly toolchain, you can panic! from the build.rs with an appropriate error message, e.g., saying that the cargo feature requires nightly Rust for now.

@Aaron1011 Aaron1011 force-pushed the Aaron1011:feature/const-fn branch from a1d1407 to add284a Oct 29, 2019
Aaron1011 and others added 2 commits Oct 29, 2019
Co-Authored-By: gnzlbg <gnzlbg@users.noreply.github.com>
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Oct 29, 2019

@gnzlbg: There are several failures, but they seem unrelated.

I'm having a really difficult time understanding how libc's test suite works, exactly what permutations of platforms and features are being tested, and whether or not I'm making my changes in a reasonable place.

build.rs Show resolved Hide resolved
Copy link
Collaborator

gnzlbg left a comment

So this looks really good. I've just left a small comment. Otherwise this LGTM.

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Oct 29, 2019

There are several failures, but they seem unrelated.

Yes, those failures are unrelated.

Aaron1011 added 2 commits Oct 29, 2019
@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Oct 29, 2019

@gnzlbg: I've added the check.

@gnzlbg
gnzlbg approved these changes Oct 29, 2019
Copy link
Collaborator

gnzlbg left a comment

LGTM, thank you @Aaron1011 !

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Nov 18, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 18, 2019

📌 Commit ca2d53e has been approved by gnzlbg

bors added a commit that referenced this pull request Nov 18, 2019
Add support for making functions `const`

PR rust-lang/rust#64906 adds the ability to write `const extern fn` and `const unsafe extern fn`, which will allow manys functions in `libc` to become `const`.

This is particuarly useful for functions which correspond to C macros (e.g. `CMSG_SPACE`). In C, these macros are constant expressions, allowing them to be used when declaring arrays. However, since the corresponding `libc` functions are not `const`, writing equivalent Rust code is impossible. Users must either perform an unecessary heap allocation, or pull in `bindgen` to evaluate the macro for specific values (e.g. `CMSG_SPACE(1)`).

However, the syntax `const extern fn` is not currently parsed by rust. To allow libc to use this without breaking backwards compatibility (i.e. bumping the minimum Rust version), I've taken the following approach:

1. A new off-by-default feature `extern-const-fn` is added to `libc`.
2. The internal `f!` macro has two versions, selected at compile-time by a `cfg_if`. When `extern-const-fn` is enabled, the declared `f!` macro passes through the `const` keyword from the macro user to the final definition (`pub const unsafe extern fn foo`. When  `extern-const-fn` is disabled, the `const` keyword passed by the macro user is discarded, resulting in a plain `pub extern const fn` being declared.

Unfortunately, I couldn't manage to get `macro_rules` to accept a normal `const` token in the proper place (after `pub`). I had to resort to placing it in curly brackets:

```rust
pub {const} fn foo(val: u8) -> i8 {
}
```

The `f!` macro then translates this to a function definition with `const` in the proper position.

I'd appreciate it if someone who's more familiar with `macro_rules!` could see if I missed a way to get the desired syntax.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 18, 2019

⌛️ Testing commit ca2d53e with merge d742eed...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 18, 2019

☀️ Test successful - checks-cirrus-freebsd-10, checks-cirrus-freebsd-11, checks-cirrus-freebsd-12, status-azure
Approved by: gnzlbg
Pushing d742eed to master...

@bors bors merged commit ca2d53e into rust-lang:master Nov 18, 2019
5 of 6 checks passed
5 of 6 checks passed
rust-lang.libc #20191029.6 failed
Details
homu Test successful
Details
nightly x86_64-unknown-freebsd-10 Task Summary
Details
nightly x86_64-unknown-freebsd-12 Task Summary
Details
rust-lang.libc (1) #20191029.6 succeeded
Details
stable x86_64-unknown-freebsd-11 Task Summary
Details
@Aaron1011 Aaron1011 deleted the Aaron1011:feature/const-fn branch Nov 18, 2019
@semarie

This comment has been minimized.

Copy link
Contributor

semarie commented Nov 20, 2019

I have a problem with this PR when running the testsuite on OpenBSD. I think it is related to my use of a stable rustc (1.39.0) to test it, but I am unsure as I think CI would not have passed (we test with stable compiler too ?).

rustc_minor_nightly() is now checking for nightly.

The run failed with thread 'main' panicked at 'Failed to get rustc version', src/libcore/option.rs:1190:5.

I added few debug println! in the code:

[libc 0.2.65] RUSTC="rustc"
[libc 0.2.65] output=Output { status: ExitStatus(ExitStatus(0)), stdout: "rustc 1.39.0\n", stderr: "" }
[libc 0.2.65] version="rustc 1.39.0\n"
[libc 0.2.65] minor=Some("39")

So it is failing on:

let nightly_raw = otry!(otry!(pieces.next()).split('-').nth(1));
@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Nov 20, 2019

So I can see why it is failing.

The output of rustc --version on OpenBSD is "rustc 1.39.0\n" and that does not contain a -, so this function returns None, and the expect fails.

We do test with many stable versions, but on the CI machines, the output of rustc -Vv is rustc 1.39.0 (4560ea788 2019-11-04). I'm not sure what the output of rustc --version is, since we don't dump it anywhere, but I expect it to be the same, and that would explain why the build.rs works there (it sees a -, and doesn't bail).

On my system, OSX, rustc --version returns rustc 1.39.0 (4560ea788 2019-11-04) as well. This makes me wonder, why does rustc --version return a different output on OpenBSD ?

In the meantime, @Aaron1011 maybe we can patch this PR, to not return None if a - is missing, and instead just return (minor, false) ?

@mati865

This comment has been minimized.

Copy link
Contributor

mati865 commented Nov 20, 2019

This makes me wonder, why does rustc --version return a different output on OpenBSD ?

Probably because it's build from tarball and not on Rust's CI. The same thing could happen with Rust installed via Linux distributions package manager.

@gnzlbg

This comment has been minimized.

Copy link
Collaborator

gnzlbg commented Nov 20, 2019

I see, thanks. Good thing you caught this @semarie !

@semarie

This comment has been minimized.

Copy link
Contributor

semarie commented Nov 20, 2019

yes, it is build from tarball.

@Aaron1011

This comment has been minimized.

Copy link
Contributor Author

Aaron1011 commented Nov 20, 2019

Let's continue the discussion at #1601, since a conversation on a merged PR isn't very discoverable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.