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

Add -Bsymbolic-functions on Linux #8864

Merged
merged 1 commit into from
Nov 10, 2014
Merged

Add -Bsymbolic-functions on Linux #8864

merged 1 commit into from
Nov 10, 2014

Conversation

staticfloat
Copy link
Member

See https://groups.google.com/forum/#!topic/julia-dev/wxYgZy70Nd0 for why this is useful, namely so that you can load libjulia.so into node and not crash.

@vtjnash
Copy link
Member

vtjnash commented Nov 5, 2014

I'm afraid it might be a bit misleading to merge this, since sys.ji may still call the wrong libuv function. fixing that would require enable the code that does two-level lookup (in the style of Darwin) for linux, whereas currently it is only is active for Windows (where we don't have the option of using flat namespaces)

@staticfloat
Copy link
Member Author

This caused Julia to call the correct function in the case of the user in the linked mailing list discussion. What's the downside to including this linker flag?

@vtjnash
Copy link
Member

vtjnash commented Nov 5, 2014

none that I know of, just that it may not be the complete story

@staticfloat
Copy link
Member Author

I'm going to merge this as it's minor, debian does it automatically, and it solves a user's problem without introducing any others that I can find. We will tackle Jameson's points when they rear their ugly, medusa-like heads.

staticfloat added a commit that referenced this pull request Nov 10, 2014
@staticfloat staticfloat merged commit cbe5ee4 into master Nov 10, 2014
@waTeim
Copy link
Contributor

waTeim commented Nov 14, 2014

Hey great, this helps me a lot. But. I saw that JLDFLAGS does not apply to building libjulia.so, but only the Julia executable.

I changed Make.inc locally to this, and got the desired result. -Bsymbolic-functions applied to the link step of julia itself is probably not necessary, but I left it to not be disruptive and just for example. I'm not sure about LDFLAGS being the right thing either, but saw that it gets applied in Makefile in src. I can do a pull request if that helps.

ifeq ($(OS), Linux)
OSLIBS += -ldl -lrt -lpthread -Wl,--export-dynamic -Wl,--version-script=$(JULIAHOME)/src/julia.expmap -Wl,--no-whole-archive $(LIBUNWIND)
JLDFLAGS = -Wl,-Bdynamic -Wl,-Bsymbolic-functions
LDFLAGS += -Wl,-Bsymbolic-functions
endif

@staticfloat
Copy link
Member Author

@tkelman This fix actually didn't fix what I thought it fixed. I thought JLDFLAGS would get applied to libjulia, but they're only applied to julia. Is it okay to throw things onto LDFLAGS or is that going to get picked up by other targets?

@tkelman
Copy link
Contributor

tkelman commented Nov 18, 2014

I think we want to avoid LDFLAGS along with CFLAGS and the like due to distribution packaging. We can skip this one for 0.3.3 if it's not fully-baked yet.

@waTeim
Copy link
Contributor

waTeim commented Nov 18, 2014

Few things

  1. Aww :(
  2. No, that's ok, because this only affects Linux that is not a PPA because it's in there already (maybe it affects Redhat, Gentoo Distros)?
  3. How about putting it in the Makefile in src instead of top level Make.inc, because it only really needs to be specifically applied to libjulia link. But does that Makefile get included any anything else)?
  4. I've tested this, and it works...

included src/Makefile

jeffw@dub1:~/src/julia-dev$ git diff src/Makefile
diff --git a/src/Makefile b/src/Makefile
index 0166694..7c8c3dd 100644
--- a/src/Makefile 
+++ b/src/Makefile
@@ -42,6 +42,10 @@ ifeq ($(USE_COPY_STACKS),1)
JCFLAGS += -DCOPY_STACKS
 endif

+ifeq ($(OS), Linux)
+LDFLAGS += -Wl,-Bsymbolic-functions
+endif
+
default: release
release debug: %: libjulia-%

and took it out of Make.inc

jeffw@dub1:~/src/julia-dev$ git diff Make.inc
diff --git a/Make.inc b/Make.inc
index 54f0c04..6c32a2c 100644
--- a/Make.inc
+++ b/Make.inc
@@ -655,7 +655,7 @@ endif

 ifeq ($(OS), Linux)
 OSLIBS += -ldl -lrt -lpthread -Wl,--export-dynamic -Wl,--version-script=$(JULIAHOME)/src/julia.expmap -Wl,--no-whole-archive $(LIBUNWIND)
-JLDFLAGS = -Wl,-Bdynamic -Wl,-Bsymbolic-functions
+JLDFLAGS = -Wl,-Bdynamic 
 endif

 ifeq ($(OS), FreeBSD)

@tkelman
Copy link
Contributor

tkelman commented Nov 18, 2014

What might be better to try would be also applying JLDFLAGS to linking libjulia, in addition to the julia executable. Not sure what, if any, downsides that might have. I wouldn't want to backport that kind of change immediately without letting it live on master for a while, give it time for at least the biggest issues to show up.

@staticfloat
Copy link
Member Author

I had the same thought. Let's do that then, and if this has to slide to
the next release, so be it.
-E

On Tue, Nov 18, 2014 at 1:02 AM, Tony Kelman notifications@github.com
wrote:

What might be better to try would be also applying JLDFLAGS to linking
libjulia, in addition to the julia executable. Not sure what, if any,
downsides that might have. I wouldn't want to backport that kind of change
immediately without letting it live on master for a while, give it time for
at least the biggest issues to show up.


Reply to this email directly or view it on GitHub
#8864 (comment).

@waTeim
Copy link
Contributor

waTeim commented Nov 18, 2014

Hrm idaknow. The flags necessary to construct libjulia.so are likely different than the executable. Take for instance -Bsymbolic-functions itself. From how I read it, it's supposed to only have any affect at all when constructing a shared library. Adding it in as a flag when constructing the executable seems strange. If you are really wanting to control from the top, maybe a new flag? I mean there's one sport that it's used anyway (ok, 2, julia-debug too).

$(build_shlibdir)/libjulia.$(SHLIB_EXT): julia.expmap $(OBJS) flisp/libflisp.a support/libsupport.a $(LIBUV)
        @$(call PRINT_LINK, $(CXXLD) $(CXXLDFLAGS) $(SHIPFLAGS) $(OBJS) $(RPATH_ORIGIN) -o $@ $(LDFLAGS) $(RELEASE_LIBS) $(SONAME)) $(CXXLDFLAGS)
        $(INSTALL_NAME_CMD)libjulia.$(SHLIB_EXT) $@
        $(DSYMUTIL) $@

^ something new here? Maybe something like LIBJLDFLAGS?

But man, I'm totally not complaining at all. You guys are awesome.

@staticfloat staticfloat deleted the sf/symbolicfunctions branch November 23, 2014 04:59
@waTeim
Copy link
Contributor

waTeim commented Nov 24, 2014

Hey guys, I have a branch I can turn into a pull request for this along the lines I proposed, interested?

@staticfloat
Copy link
Member Author

Yes please! Submit a PR and we'll hash out the details there.

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