Skip to content

Compilation fixes for iOS #10227

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 1 commit into from
Closed

Compilation fixes for iOS #10227

wants to merge 1 commit into from

Conversation

kud1ing
Copy link

@kud1ing kud1ing commented Nov 1, 2013

No description provided.

@kud1ing
Copy link
Author

kud1ing commented Nov 1, 2013

_NSGetEnviron() is private and can not be called on iOS, crt_externs.h is not available.

I am not sure about .align 2. Apple's tools do not accept .align without an argument.
I think this document implies, that the default value should be 2: http://stuff.mit.edu/afs/athena/project/rhel-doc/3/rhel-as-en-3/arm-directives.html

@adrientetar
Copy link
Contributor

I'd just drop the .align conditional then since 2 is the default argument.

Also, why not replace with .align 2 in morestack.S too? This way all arm asm files have the fix.

@kud1ing
Copy link
Author

kud1ing commented Nov 4, 2013

Regarding morestack.S: i've fixed the files where i encountered an error.
I have not met this file yet.

@alexcrichton
Copy link
Member

Would you mind adding some comments in the assembly as to what the align directives are doing? Things like this are always good to have a reason as to why they exist in the first place.

Also, I've found that xcrun doesn't have a --show-sdk-path argument, so I'm always getting lots of make errors printed whenever I do anything. Do you know if this changed in a recent version of OSX? It would be great to stop causing these errors.

@kud1ing
Copy link
Author

kud1ing commented Nov 4, 2013

I've only tried with XCode 5 (which depends on OS X 10.9) and it's present there: https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/xcrun.1.html

We could try to support older XCode/Mac OS X combinations, but since both are free nowadays i am not sure whether it's worth making things more complicated. What do you think?

@kud1ing
Copy link
Author

kud1ing commented Nov 4, 2013

@alexcrichton: Ah, sorry. You meant, you get the errors, even when not compiling for iOS/ARM?

Do xcrun --show-sdk-platform-path or xcode-select -p work for you?
Or how about the pragmatic xcrun --show-sdk-path 2>/dev/null?

@alexcrichton
Copy link
Member

Looks like xcode-select -p prints something relevant, but I would also be ok with the 2> /dev/null idea. It looks like this is a relic of 4.6.3, so I'm ok just piping the error elsewhere and then requiring 5.0 (if that's necessary).

@kud1ing
Copy link
Author

kud1ing commented Nov 5, 2013

@alexcrichton: Ok, can you confirm that 2>/dev/null hides the error messages for you?

@alexcrichton
Copy link
Member

It does indeed.

@kud1ing
Copy link
Author

kud1ing commented Nov 5, 2013

@adridu59: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0489c/Babecdje.html says Use ALIGN to take advantage of caches on some ARM processors, but this does not seem to be supported by Apple tools. I think .align 2 makes sense for iOS.

The original .align was written by @kyeongwoon, maybe he can give some input.

@adrientetar
Copy link
Contributor

My point is that since these docs seem to say that .align == .align 2 (2 is default), I'd suggest to use .align 2 for everyone, without ifdefs.

@kud1ing
Copy link
Author

kud1ing commented Nov 6, 2013

I think this PR should go in as is at this point in time:

  • /usr/include should not be in the include path, the SDK has its correct version of /usr/include
  • many projects solve the _NSGetEnviron() issue for iOS as we do above. It is in line with how we handle it for FreeBSD
  • I've added 2>/dev/null to silence the error messages on systems with older XCode
  • the alignment issue is not 100% clear to me. The current version allows us to go on and investigate compilation/linking problems further down. We should come back to this and I would be more than happy if Samsung (the original authors) could give some input here. The worst case is that incorrect alignment will get us runtime crashes when running Rust-compiled iOS executables.

@alexcrichton: r?

@kud1ing
Copy link
Author

kud1ing commented Nov 6, 2013

What a pitty, it needs to be defined(TARGET_OS_IPHONE) => !(TARGET_OS_IPHONE)in src/rt/rust_builtin.cpp.
Will fix it in 8 hours.

@kud1ing
Copy link
Author

kud1ing commented Nov 6, 2013

Fixed the TARGET_OS_IPHONE check. The include path additions in mk/rt.mk slipped in by accident, but it does not hurt. @alexcrichton r?

@@ -28,6 +28,13 @@ LIBUV_FLAGS_i386 = -m32 -fPIC -I$(S)src/etc/mingw-fix-include
LIBUV_FLAGS_x86_64 = -m64 -fPIC
ifeq ($(OSTYPE_$(1)), linux-androideabi)
LIBUV_FLAGS_arm = -fPIC -DANDROID -std=gnu99
else ifeq ($(OSTYPE_$(1)), apple-darwin)
ifeq ($(HOST_$(1)), arm)
IOS_SDK := $(shell xcrun --show-sdk-path -sdk iphoneos)
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this also need 2>/dev/null?

@alexcrichton
Copy link
Member

Hm, the number of commits is growing a bit too much, would you mind rebasing into one commit and adding some more description to the commit comments about what's going on? A commit message of "fix target" isn't really too useful when we're tracing back through the history.

@adrientetar
Copy link
Contributor

@kud1ing Historically, the align directive is there because ARM SoCs used to work only on aligned mode. Nowadays they support unaligned code but it's still not something common.

@alexcrichton
Copy link
Member

I think there's still one spot which needs 2>/dev/null

- remove /usr/include from the include path since the iOS SDK provides the correct version
- `_NSGetEnviron()` is private and not available on iOS
- `.align` without an argument is not allowed with the Apple tools. 2^2 should be the default alignment
- ignore error messages for XCode < 5
- pass include path to libuv
@kud1ing
Copy link
Author

kud1ing commented Nov 6, 2013

rebased

bors added a commit that referenced this pull request Nov 6, 2013
@bors bors closed this Nov 6, 2013
@kud1ing kud1ing deleted the ios branch November 7, 2013 06:22
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