Skip to content

Add SGX target #1124

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

Closed
wants to merge 3 commits into from
Closed

Add SGX target #1124

wants to merge 3 commits into from

Conversation

jethrogb
Copy link

This adds support for the x86_64-fortanix-unknown-sgx target. See rust-lang/rust#56066 for details.

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

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 19, 2018

r? @gnzlbg

@rust-highfive rust-highfive assigned gnzlbg and unassigned alexcrichton Nov 19, 2018
Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

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

Modulo nitpicks LGTM.

Is there a way to run libc-test on this target ?

@alexcrichton
Copy link
Member

It's sort of a longstanding bug that the libc crate has so many function definitions in its root because they're not always available. Ideally these functions should (historically) have been pushed down into the various other modules. That requires duplication, but that's ok with the libc crate!

Would you be ok pushing all the functions into lower modules (duplicating them) and that way the top module should only have types shared by all targets?

@jethrogb
Copy link
Author

Would you be ok pushing all the functions into lower modules (duplicating them) and that way the top module should only have types shared by all targets?

Maybe, that seems contrary to efforts like #1086

@alexcrichton
Copy link
Member

Hm I'm not sure I understand, how's it contrary to c_void and such?

@jethrogb
Copy link
Author

Sorry, I specifically meant the de-duplication that was done as part of that PR

@alexcrichton
Copy link
Member

Oh I just mean duplicating the definition, for any one platform it's still just defined once. (this is also just functions too I'm talking about as sgx has all the types it looks like)

@jethrogb
Copy link
Author

Ok, I'd like to see #1126 land first though.

@jethrogb
Copy link
Author

Made the changes suggested by @alexcrichton. This is now based on #1126.

@jethrogb jethrogb force-pushed the jb/sgx-target branch 2 times, most recently from 626edb3 to 3ef2444 Compare November 20, 2018 08:46
@jethrogb
Copy link
Author

CI failure for nightly is the same as #1126

@bors
Copy link
Contributor

bors commented Nov 20, 2018

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

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 20, 2018

This appears to contain an spurious commit :/

@jethrogb
Copy link
Author

jethrogb commented Nov 20, 2018

GitHub is broken. d145731 is in master now, git should be able to figure it out when merging. You can view the changes from this PR at https://github.com/rust-lang/libc/compare/master...jethrogb:jb/sgx-target?expand=1

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 20, 2018

This crate duplicates a lot of code just to be able to include some ctypes for a target without libc.

Do you think it would be ok to just make libc silently compile for this target like we do for the other targets that do not have a libc, and then add the ctypes somewhere else ? If you need them in libstd, you can add them there, or you can add a dependency for libstd that imports a crate that contains them for your target (libstd already has some target-dependent dependencies on macos and linux).

@alexcrichton
Copy link
Member

Duplication is the bread and butter of the libc crate and has been tried and true for quite some time now. I do realize it's pretty backwards compared to how we normally work, but it really is the easiest thing to maintain in this crate, and these are the "weirdest" PRs where items are pushed down to lower platforms.

I am personally in favor of this PR and would like to merge. If there's a standard C compiler for targets then that means we have type definitions, but it may mean that there's no C functions themselves (such as for this target and wasm)

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 20, 2018

Duplication is the bread and butter of the libc crate and has been tried and true for quite some time now.

I kind of wish we would just put all this code which is shared by all libcs except sgx in a single place instead of duplicating it.

@alexcrichton alexcrichton mentioned this pull request Nov 20, 2018
@bors
Copy link
Contributor

bors commented Nov 21, 2018

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

@alexcrichton
Copy link
Member

I think this was effectively added in #1129, so closing!

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