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.

Sign up for GitHub

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

Jump to bottom

Add BOTHER & termios2 to Android & Linux #711

Merged
merged 2 commits into from
Oct 19, 2017
Merged

Add BOTHER & termios2 to Android & Linux #711

merged 2 commits into from
Oct 19, 2017

Conversation

Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Aug 1, 2017

This should be the same for both the struct and the constant across all Linux/Android platforms, but we'll see!

@Susurrus Susurrus force-pushed the termios2 branch 3 times, most recently from 32948a9 to 68463c5 Compare August 2, 2017 02:41
@Susurrus Susurrus changed the title WIP: Add BOTHER and termios2 to Android & Linux Add BOTHER to Android & Linux Aug 2, 2017
@alexcrichton
Copy link
Member

A whitelist is added for this but is it verified anywhere?

@Susurrus
Copy link
Contributor Author

Susurrus commented Aug 2, 2017

What do you mean verified? I looked through the Linux sources to figure out these values and they should be correct.

@alexcrichton
Copy link
Member

Oh I just mean verified in the sense of libc-test/build.rs. If it's ignored there on all linux platforms, and the value varies across architectures on linux platforms, how do we know it's right?

@Susurrus
Copy link
Contributor Author

Susurrus commented Aug 8, 2017

@alexcrichton I looked at the Linux sources to figure out what these values are. For situations like this, what is the bar for proving this is correct? I haven't had to do it for CMSPAR when I added it, so I'm not certain what you're looking for.

@alexcrichton
Copy link
Member

Constants like CMSPAR are only ignored on mips-unknown-linux-gnu it looks like, meaning it's verified on almost all platforms. BOTHER, however, is validated nowhere.

Can this constant not be accessed from C code? If not why not, and how is it supposed to be used in C? If so, how come we can't access it that way?

@Susurrus
Copy link
Contributor Author

Susurrus commented Aug 9, 2017

BOTHER is only defined in asm/termbits.h, which cannot be included along with termios.h. To test this we'd need a separate test that only included this header to verify the value. There are various posts on the internet that ask how you're supposed to use the termios2 API since it conflicts with termios1, and the response is normally to inline the necessary datatypes to work around the conflict.

@alexcrichton
Copy link
Member

I'd be pretty uncomfortable adding this with no verification on any platform basically. I think we need to verify it somewhere automatically to at least enusre it's correct on some platforms. This may require changes to ctest perhaps to generate more than one object file.

@Susurrus Susurrus mentioned this pull request Aug 10, 2017
Merged
@bors
Copy link
Contributor

bors commented Aug 11, 2017

☔ The latest upstream changes (presumably #722) made this pull request unmergeable. Please resolve the merge conflicts.

@Susurrus
Copy link
Contributor Author

I'd also like to include NCCS and struct termios2 from the Linux asm/termbits.h header, but I'm uncertain how to do this. NCCS is already defined in termios.h and is a different value between these two headers on most platforms. There's no const_name method within ctest to allow setting it to NCCS_TERMIOS2 in the source and still allowing it to be translated to just NCCS for testing.

I could for now at least support the struct without this constant, though that's definitely less than ideal.

@Susurrus
Copy link
Contributor Author

Note that this branch is now based on #720 as that's needed to implement testing.

@bors
Copy link
Contributor

bors commented Aug 25, 2017

☔ The latest upstream changes (presumably #720) made this pull request unmergeable. Please resolve the merge conflicts.

@Susurrus
Copy link
Contributor Author

@alexcrichton I want to rebase and push this forward, but I'd like to add the termios2 struct to it. That strict is defined using a different value for the NCCS constant. Is it fine to name it NCCS_termios2 and exclude it from testing, as the size of the struct will be checked by the testing code.

@alexcrichton
Copy link
Member

Sure yeah sounds reasonable, I'd probably just not expose the constant if possible

@Susurrus
Copy link
Contributor Author

Just rebased and updated this now that #720 landed. One thing I wasn't able to do was to test the VINTR and related constantsto check they have the proper values in both the linux_fcntl and main test binaries. It looks like those binaries are linked together as part of the final test suite, which means there are linking errors about test functions when the same constants are tested in both binaries. These should be the same, but I thought an extra check would be nice so I wanted to mention this issue.

@Susurrus
Copy link
Contributor Author

I'm not certain why there're failures on mips targets, because the declaration of termios follows pretty closely to termios2, which passes all CI tests. I think it might be in the type name mapping, which i don't really understand. Any idea @alexcrichton?

alexcrichton
alexcrichton reviewed Aug 25, 2017
View reviewed changes
libc-test/build.rs Outdated
// The termios2 struct is the same as `termios` on powerpc, so it's actually
// undeclared in the headers. We'll still expose it there to still provide
// some type safety, but we can't check it directly.
"termios2" if !powerpc => false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this use struct_name to rename the struct to assert termios2 is the same as termios on powerpc?

@alexcrichton
Copy link
Member

This just looks like c_cc has a different type in the struct than is defined in Rust?

@bors
Copy link
Contributor

bors commented Aug 30, 2017

☔ The latest upstream changes (presumably #747) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

I'm gonna close this due to inactivity, but feel free to reopen with a rebase!

@Susurrus
Copy link
Contributor Author

Looks like I can't reopen this PR, but I'm definitely interested in merging it.

@Susurrus Susurrus changed the title Add BOTHER to Android & Linux Add BOTHER & termios2 to Android & Linux Oct 19, 2017
@Susurrus
Copy link
Contributor Author

@alexcrichton This crashes for me with really weird errors when trying to rename termios2 to termios on powerpc platforms, but I figured I'd submit it, see what CI says, and maybe get some help.

@Susurrus Susurrus force-pushed the termios2 branch 4 times, most recently from d87d24e to e95e79e Compare October 19, 2017 03:53
Bryant Mairs added 2 commits October 18, 2017 20:57
@Susurrus
Add BOTHER to Android & Linux
12cfa1e
@Susurrus
Add termios2 for Android & Linux
6d55c24 
Note that termios2 doesn't exist on powerpc(64), termios
is used instead.
@Susurrus
Copy link
Contributor Author

Looks like a spurious failure on aarch64-unknown-linux-gnu, so if this looks good should be fine to merge.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Oct 19, 2017

📌 Commit 6d55c24 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 19, 2017

⌛ Testing commit 6d55c24 with merge 9a9f71e...

bors added a commit that referenced this pull request Oct 19, 2017
@bors
Auto merge of #711 - Susurrus:termios2, r=alexcrichton
9a9f71e 
Add BOTHER & termios2 to Android & Linux

This should be the same for both the struct and the constant across all Linux/Android platforms, but we'll see!
@bors
Copy link
Contributor

bors commented Oct 19, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 9a9f71e to master...

@bors bors merged commit 6d55c24 into rust-lang:master Oct 19, 2017
danielverkamp pushed a commit to danielverkamp/libc that referenced this pull request Apr 28, 2020
@rbirdic @gnzlbg
Fix MIPS build after rust-lang#711
29d1f2e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Susurrus @alexcrichton @bors @Mark-Simulacrum