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 flock64 to linux_like platforms #1561

Merged
merged 3 commits into from
Nov 25, 2019
Merged

Conversation

elichai
Copy link
Contributor

@elichai elichai commented Oct 18, 2019

Hi,
I'm not sure about what to make of the ifdef in glibc __USE_LARGEFILE64 that covers it.
but I copied the impl from glibc's headers.
everything is the same except for sparc which has a reserved short int.
and in musl it's just #define flock64 flock

@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 @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.

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 18, 2019

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 18, 2019

📌 Commit 0da3a5f has been approved by gnzlbg

@glaubitz
Copy link
Contributor

If this gets merged before my sparc PR, I should probably add flock64 to sparc as well.

@bors
Copy link
Contributor

bors commented Nov 18, 2019

⌛ Testing commit 0da3a5f with merge 1d883da...

bors added a commit that referenced this pull request Nov 18, 2019
Add flock64 to linux_like platforms

Hi,
I'm not sure about what to make of the ifdef in glibc `__USE_LARGEFILE64` that covers it.
but I copied the impl from glibc's headers.
everything is the same except for sparc which has a reserved short int.
and in musl it's just `#define flock64 flock`
@bors
Copy link
Contributor

bors commented Nov 19, 2019

💔 Test failed - status-azure

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 20, 2019

From the error messages it appears that the type is defined differently on different targets. You need to fix those.

@elichai
Copy link
Contributor Author

elichai commented Nov 20, 2019

From the error messages it appears that the type is defined differently on different targets. You need to fix those.

First of all awesome test suite :) I need to read how it works.

Second, seems that the problem is the musl targets, which makes sense. i'll fix it.

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 20, 2019

Notice that there are generic files for linux/gnu/mod.rs and linux/musl/mod.rs, so you might get away with just having two structs (I'm not sure).

@elichai
Copy link
Contributor Author

elichai commented Nov 20, 2019

Notice that there are generic files for linux/gnu/mod.rs and linux/musl/mod.rs, so you might get away with just having two structs (I'm not sure).

the glibc once are fine (they match glibc correctly and pass the tests) on the other hand, musl have
#define flock64 flock and even that it's under #if defined(_LARGEFILE64_SOURCE) || defined(_GNU_SOURCE) so maybe there's just no flock64 in the test framework, although the errors are weird. I'm trying to reproduce them locally

https://git.musl-libc.org/cgit/musl/tree/include/fcntl.h#n202

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 20, 2019

The test framework always defines _GNU_SOURCE for all linux targets: https://github.com/rust-lang/libc/blob/master/libc-test/build.rs#L2074

@glaubitz
Copy link
Contributor

glaubitz commented Nov 20, 2019 via email

@elichai
Copy link
Contributor Author

elichai commented Nov 20, 2019

Be careful though as flock64 is different on sparc* targets so you need arch-specific definitions.

I know, did that already, the weird thing is actually musl targets which should be flock==flock64

@elichai
Copy link
Contributor Author

elichai commented Nov 20, 2019

@gnzlbg Ok, I got it down to this example: https://ideone.com/rgk7OZ
the problem seems to be line 22:

  typedef struct {
    unsigned char c;
    flock v;
  } type;

it should be struct flock v;

The question is why this happens

@elichai
Copy link
Contributor Author

elichai commented Nov 20, 2019

Update, that's definitely because I defined pub type flock64 = ::flock;. it seems like the translation unit to C doesn't understand it properly.
i'll try to reproduce on ctest and open an issue there.

Should I replace the typedef with a duplicate struct?

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 20, 2019

There are methods in ctest to configure how to name the type aliases, you can use cfg.type_name to tell ctest to expand flock64 on that particular target to struct flock64 - there should be many examples of this in the libc-test/build.rs.

libc-test/build.rs Outdated Show resolved Hide resolved
libc-test/build.rs Outdated Show resolved Hide resolved
Co-Authored-By: gnzlbg <gnzlbg@users.noreply.github.com>
@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 25, 2019

Thanks! @bors: r+

@bors
Copy link
Contributor

bors commented Nov 25, 2019

📌 Commit 490e073 has been approved by gnzlbg

@bors
Copy link
Contributor

bors commented Nov 25, 2019

⌛ Testing commit 490e073 with merge 7f33bcb...

bors added a commit that referenced this pull request Nov 25, 2019
Add flock64 to linux_like platforms

Hi,
I'm not sure about what to make of the ifdef in glibc `__USE_LARGEFILE64` that covers it.
but I copied the impl from glibc's headers.
everything is the same except for sparc which has a reserved short int.
and in musl it's just `#define flock64 flock`
@bors
Copy link
Contributor

bors commented Nov 25, 2019

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

@bors bors merged commit 490e073 into rust-lang:master Nov 25, 2019
@glaubitz
Copy link
Contributor

Oh, is the CI working again? I just re-pushed my PR.

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.

5 participants