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

iOS stdlib / universal library regression #34617

Closed
TimNN opened this issue Jul 2, 2016 · 10 comments
Closed

iOS stdlib / universal library regression #34617

TimNN opened this issue Jul 2, 2016 · 10 comments
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.

Comments

@TimNN
Copy link
Contributor

TimNN commented Jul 2, 2016

Originally reported at TimNN/cargo-lipo#3.

The following issue occurs on nightly-2016-07-02 but not on nightly-2016-06-24, I suspect the regression was caused by #34055. Related: #29664.

Creating a universal library for all iOS targets and linking it in XCode produces the following warnings when targeting a simulator, but not when targeting the real device:

ld: warning: URGENT: building for iOS simulator, but linking in object file (/*/rusty/target/universal/debug/librusty.a(powidf2.c.o)) built for OSX. Note: This will be an error in the future.
ld: warning: object file (/*/rusty/target/universal/debug/librusty.a(powidf2.c.o)) was built for newer OSX version (10.7) than being linked (9.3)
ld: warning: URGENT: building for iOS simulator, but linking in object file (/*/rusty/target/universal/debug/librusty.a(powisf2.c.o)) built for OSX. Note: This will be an error in the future.
ld: warning: object file (/*/rusty/target/universal/debug/librusty.a(powisf2.c.o)) was built for newer OSX version (10.7) than being linked (9.3)

As I understand the situation:

  • these warnings are reported for two object files which were included in the universal library as part of compiler-rt
  • these warnings can be fixed by passing -mios-simulator-version-min=7.0 to the appropriate tools
  • this flags seems to be no longer passed around when building compiler-rt, however I believe that the standard library is still build with that flag because there would be more warnings / errors otherwise
  • searching PR's merged after nightly-2016-06-24 for compiler-rt makes Convert makefiles to build LLVM/compiler-rt with CMake #34055 look like the likeliest candidate to have caused this regression (sadly there are no nightlies available between the two, which makes narrowing it down further very inconvenient).
@TimNN
Copy link
Contributor Author

TimNN commented Jul 2, 2016

I did a local build and reverting 59db95b did indeed fix the problem.

@TimNN
Copy link
Contributor Author

TimNN commented Jul 3, 2016

After some more digging & experimentation I believe the current situation is as follows:

  • the current ios (simulator) specific C(XX) flags are explicitly not passed to the compiler-rt build
  • the compiler-rt (cmake) build system can figure out the -mios-min-version=* flags itself
  • contrary to the make bases build system, the cmake based buildsystem does not handle the -mios-simulator-version-min=* flag

@alexcrichton
Copy link
Member

Gah this is almost guaranteed to be related to the PR that switched to CMake. It was also the reason we didn't have nightlies for so long!

So as I think you've discovered we actively don't pass cflags to the ios builds of compiler-rt because compiler-rt is a little over-aggressive and applies a ton of its own flags to the build, which I thought included the ios ones but apparently don't! Unfortunately builds will fail if we pass -mios-min-version through CMAKE_C_FLAGS because I think it conflicts with this block which attempts to pass through mmacosx-version-min.

Note that ios support in the cmake build system of compiler-rt seems experimental at best, so it sounds like to fix this we're gonna have to patch something rather than continue to jump through hoops for compiler-rt.

@alexcrichton alexcrichton added A-build regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Jul 3, 2016
@TimNN
Copy link
Contributor Author

TimNN commented Jul 3, 2016

Yeah, thats what I figured. I sadly don't have much experience with either the whole iOS compiling / linking intricacies or cmake / makefiles.

I've been attempting to fiddle with the compiler-rt cmake files a little but so far no success.

However the compiler-rt CMake / iOS (Simulator) build seems to be definitely broken, since a build for the simulator target x86_64-apple-ios compiles the source files with

cd /Users/logic/Projects/rustc/dev/x86_64-apple-ios/rt/compiler-rt/lib/builtins &&
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang 
  -O3 -DNDEBUG -arch x86_64 -isysroot
  /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk
  -Oz -Wall -fomit-frame-pointer -ffreestanding -arch x86_64 -fPIC
  -o CMakeFiles/clang_rt.hard_pic_x86_64_macho_embedded.dir/x86_64/floatdidf.c.o  
  -c /Users/logic/Projects/rustc/dev/src/compiler-rt/lib/builtins/x86_64/floatdidf.c

(notice how the MacOSX SDK is used, the correctone would be iPhoneSimulator.platform I believe).

Targeting a "real" iOS architecture produces:

cd /Users/logic/Projects/rustc/dev/aarch64-apple-ios/rt/compiler-rt/lib/builtins &&
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang
  -O3 -DNDEBUG -arch arm64    -isysroot
  /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS9.3.sdk
  -miphoneos-version-min=6.0 -fPIC -O3 -fvisibility=hidden -DVISIBILITY_HIDDEN -Wall -fomit-frame-pointer -arch arm64
  -o CMakeFiles/clang_rt.builtins_arm64_ios.dir/floatdidf.c.o 
  -c /Users/logic/Projects/rustc/dev/src/compiler-rt/lib/builtins/floatdidf.c

@TimNN
Copy link
Contributor Author

TimNN commented Jul 3, 2016

So I managed a patch for compiler-rt which works around the issue. This is probably not the correct way of fixing things and I don't know if the patch has any side effects (I believe it should only modify iOS simulator targets but who knows?).

I have not yet done a complete build of rust with this patch (only compiler-rt), I'll try to do a complete build tomorrow.

The patch can be found at rust-lang/compiler-rt@rust-2016-06-16...TimNN:fixing-stuff.

@alexcrichton
Copy link
Member

@TimNN that looks good to me!

@TimNN
Copy link
Contributor Author

TimNN commented Jul 4, 2016

What is the process of integrating that patch into (the rust fork of) compiler-rt?

I plan on adding some documentation to the patch / commit and doing a full build of rust as a last test later today.

Also, while this patch works for ios(sim), it probably breaks stuff for watchos / tvos (which hopefully doesn't matter that much since they are not supported (at least not officially) as far as I know).

TimNN added a commit to TimNN/compiler-rt that referenced this issue Jul 4, 2016
--- Workaround ---
The REF_OS variable was introduced to workaround linking problems when
compiler-rt is build for the iOS simulator.

Without this workaround, trying to link compiler-rt into an executable for
the iOS simulator would produce an error like

  ld: warning: URGENT: building for iOS simulator, but linking in object
  file built for OSX. Note: This will be an error in the future.

The underlying reason is that the iOS simulator specific configuration is
stored in variables named like DARWIN_iossim_SYSROOT and not
DARWIN_macho_embedded_SYSROOT. Thus, with the current setup, compiler-rt
would be compiled against the OS X SDK and not the iPhone Simulator SDK.

As a workaround we manually override macho_embedded with iossim when
accessing the DARWIN_*_SYSROOT and DARWIN_*_BUILTIN_MIN_VER_FLAG variables.

This workaround probably break builds of compiler-rt for the watchOS and
tvOS simulators (if they weren't broken already).

See also rust-lang/rust#34617.
@TimNN
Copy link
Contributor Author

TimNN commented Jul 4, 2016

@alexcrichton I (force) pushed a new commit to the linked branch which adds a bit of documentation to the workaround.

I also did full local build of rust and verified that the patch fixes the original problem.

The patch commit is rust-lang/compiler-rt@2ab4911.

alexcrichton pushed a commit to rust-lang/compiler-rt that referenced this issue Jul 4, 2016
--- Workaround ---
The REF_OS variable was introduced to workaround linking problems when
compiler-rt is build for the iOS simulator.

Without this workaround, trying to link compiler-rt into an executable for
the iOS simulator would produce an error like

  ld: warning: URGENT: building for iOS simulator, but linking in object
  file built for OSX. Note: This will be an error in the future.

The underlying reason is that the iOS simulator specific configuration is
stored in variables named like DARWIN_iossim_SYSROOT and not
DARWIN_macho_embedded_SYSROOT. Thus, with the current setup, compiler-rt
would be compiled against the OS X SDK and not the iPhone Simulator SDK.

As a workaround we manually override macho_embedded with iossim when
accessing the DARWIN_*_SYSROOT and DARWIN_*_BUILTIN_MIN_VER_FLAG variables.

This workaround probably break builds of compiler-rt for the watchOS and
tvOS simulators (if they weren't broken already).

See also rust-lang/rust#34617.
@alexcrichton
Copy link
Member

What is the process of integrating that patch into (the rust fork of) compiler-rt?

Ah it's pretty easy, just sending a PR to the right branch of rust-lang/compiler-rt

Also, while this patch works for ios(sim), it probably breaks stuff for watchos / tvos (which hopefully doesn't matter that much since they are not supported (at least not officially) as far as I know).

Yeah that's fine, no worries there.

I (force) pushed a new commit to the linked branch which adds a bit of documentation to the workaround.

Awesome! I've gone ahead and cherry-picked that into our branch. Want to send a PR to rust-lang/rust updating the compiler-rt submodule?

@TimNN
Copy link
Contributor Author

TimNN commented Jul 4, 2016

Sure, I'll send the PR to update the submodule.

bors pushed a commit that referenced this issue Jul 5, 2016
bors added a commit that referenced this issue Jul 5, 2016
Update compiler-rt with iOS linking warnings workaround

Closes #34617.

r? @alexcrichton
alexcrichton pushed a commit to rust-lang/compiler-rt that referenced this issue Jul 15, 2016
--- Workaround ---
The REF_OS variable was introduced to workaround linking problems when
compiler-rt is build for the iOS simulator.

Without this workaround, trying to link compiler-rt into an executable for
the iOS simulator would produce an error like

  ld: warning: URGENT: building for iOS simulator, but linking in object
  file built for OSX. Note: This will be an error in the future.

The underlying reason is that the iOS simulator specific configuration is
stored in variables named like DARWIN_iossim_SYSROOT and not
DARWIN_macho_embedded_SYSROOT. Thus, with the current setup, compiler-rt
would be compiled against the OS X SDK and not the iPhone Simulator SDK.

As a workaround we manually override macho_embedded with iossim when
accessing the DARWIN_*_SYSROOT and DARWIN_*_BUILTIN_MIN_VER_FLAG variables.

This workaround probably break builds of compiler-rt for the watchOS and
tvOS simulators (if they weren't broken already).

See also rust-lang/rust#34617.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-nightly Performance or correctness regression from stable to nightly.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants