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

Xcode 7.3 thread local storage bug #938

Closed
ilovezfs opened this issue Apr 17, 2016 · 15 comments
Closed

Xcode 7.3 thread local storage bug #938

ilovezfs opened this issue Apr 17, 2016 · 15 comments

Comments

@ilovezfs
Copy link

ilovezfs commented Apr 17, 2016

Building igraph-0.7.1 with Xcode 7.3, I have to disable thread local storage, passing --enable-tls=no to configure, otherwise the build fails with

ld: section __DATA/__thread_bss extends beyond end of file, file '.libs/libigraph_la-bignum.o' for architecture x86_64

This is an upstream llvm bug: https://llvm.org/bugs/show_bug.cgi?id=27059

However, I believe it can be mitigated if you reduce the size of the arrays using tls, as we did here:
https://github.com/Homebrew/homebrew-science/commit/62088d6ec6b6d7aa03a3534877053196650fe1e0

Hopefully llvm will fix this soon, but a patch for igraph in the meantime would be great if possible.

@ntamas
Copy link
Member

ntamas commented Apr 17, 2016

This has recently been brought to our attention on our mailing list - are you the same person by any chance?

I'll try the workaround you mentioned soon.

@ilovezfs
Copy link
Author

ilovezfs commented Apr 17, 2016

Nope, not the same person. Just stumbled into it today working through some reverse dependencies in homebrew/science. I'd seen the same bug before with metis. This is the PR that corresponds to the commit I mentioned above: https://github.com/Homebrew/homebrew-science/pull/3491

@ntamas
Copy link
Member

ntamas commented Apr 18, 2016

Which version of the compiler it is and what flags are you passing to ./configure? I have tried to reproduce this on my machine (OS X 10.11.1, XCode 7.3) but it seems that the compilation went just fine. clang -v says this:

Apple LLVM version 7.3.0 (clang-703.0.29)
Target: x86_64-apple-darwin15.0.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

@ilovezfs
Copy link
Author

@ilovezfs
Copy link
Author

Looks like it wants -g and -O2 or it happens.

@ilovezfs
Copy link
Author

Applying this diff to Homebrew allows it to build

iMac-TMP:Library joe$ git diff
diff --git a/Library/ENV/4.3/cc b/Library/ENV/4.3/cc
index e7ab650..18e6b2e 100755
--- a/Library/ENV/4.3/cc
+++ b/Library/ENV/4.3/cc
@@ -149,9 +149,9 @@ class Cmd
     args = []

     case arg
-    when /^-g\d?$/, /^-gstabs\d+/, "-gstabs+", /^-ggdb\d?/,
+    when /^-gstabs\d+/, "-gstabs+", /^-ggdb\d?/,
       /^-march=.+/, /^-mtune=.+/, /^-mcpu=.+/,
-      /^-O[0-9zs]?$/, "-fast", "-no-cpp-precomp",
+      "-fast", "-no-cpp-precomp",
       "-pedantic", "-pedantic-errors", "-Wno-long-double",
       "-Wno-unused-but-set-variable"
     when "-fopenmp", "-lgomp", "-mno-fused-madd", "-fforce-addr", "-fno-defer-pop",

Unfortunately, building with -Os instead of -g -O2 shouldn't cause the build to fail, but apparently it does right now.

@ilovezfs
Copy link
Author

Just to confirm further, I can reproduce it outside of Homebrew if I replace the -O2 -g in your configure with -Os.

@ntamas
Copy link
Member

ntamas commented Apr 18, 2016

Okay, I managed to reproduce this. Changing BN_MAXSIZE in bignum.h to 202 (original value: 512) resolves this. BN_MAXSIZE = 203 triggers the error again.

@ntamas
Copy link
Member

ntamas commented Apr 18, 2016

@gaborcsardi will we ever need to work with bignums larger than 6464 bits? If not, I'm inclined to change this unconditionally to 202 because it would be hard to figure out the exact clang versions for which this workaround is required.

@gaborcsardi
Copy link
Contributor

202 is fine I guess.

@ntamas ntamas closed this as completed in 0387a58 Apr 18, 2016
@ilovezfs
Copy link
Author

ilovezfs commented Apr 19, 2016

The value needed for a successful workaround actually seems not to be universal.

Building igraph-0.7.1, I had to set
156 for -Os (no -g); >= 157 breaks.
220 for -O2 (no -g); >=221 breaks

For HEAD, the 202 you hard coded works with -Os and it seems I can crank it up way beyond the original 512.

@ntamas
Copy link
Member

ntamas commented Apr 19, 2016

Hmmm, I think in the end it all boils down to the total size of the __DATA/__thread_bss segment in the linked library - if it is above a certain size, the build breaks, and in the end the total size of the segment might depend on the exact compiler flags being used.

If we want to be on the safe side, I can lower it to, say, 128. That would still give us 4096 bits for bignums, which should be plenty, and then it's also a power of two, which might help with alignment for the bit arrays we use for the bignums. What do you think?

@ilovezfs
Copy link
Author

Yeah that sounds like what's probably going on. I'm wondering if maybe a 0.7.2 should be tagged since it seemed that this doesn't affect HEAD for some reason?

@ntamas
Copy link
Member

ntamas commented Apr 25, 2016

I have lowered BN_MAXSIZE further down to 128.

@gaborcsardi, are there any plans to make a 0.8.0 or 0.7.2 release of the C core in the near future?

@ilovezfs
Copy link
Author

@ntamas Thanks. I've merged that workaround for now and rebuilt the distributed binaries: https://github.com/Homebrew/homebrew-science/pull/3587

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

No branches or pull requests

3 participants