-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Users can decide whether to set RPATH or RUNPATH #9168
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start -- see my comment below on ld
vs ccld
.
The other thing that should be handled here is if the build provides --enable-new-dtags
. We need to strip that, because I dunno what happens if both --enable-new-dtags
and --disable-new-dtags
are provided together on the link line. We want disable to be all that is left.
Ready for a second review |
lib/spack/env/cc
Outdated
@@ -325,6 +343,8 @@ while [ -n "$1" ]; do | |||
die "-Wl,-rpath was not followed by -Wl,*" | |||
fi | |||
rp="${arg#-Wl,}" | |||
elif [[ "$arg" = --enable-new-dtags ]] ; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading bash
is not my best skill, but I think we are currently not treating NAG the same as the other compilers:
- RPATHs are not collected in
rp
as there's no condition that matches-Wl,-Wl,,-rpath,,XXX
- Every
-Wl,-Wl,,-rpath,,XXX
will be appended toother_args
Am I misreading something? In case: do we want to fix this here or, having the issue a minimal impact, can we defer it to a later PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish I understood the NAG linker argument syntax there -- I wouldn't mind actually fixing this here if you're up for it. I just don't have a NAG compiler to test on. I looked at the NAG manual and it doesn't seem to indicate that we need two -Wl,
's. Do you know who we could ask? I suspect we don't have many NAG users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @adamjstewart is our best bet. As far as I remember he had to install NAG in his previous job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAG calls gcc for linking, which calls ld. Therefore, when we specify a flag with a single prefix -Wl for NAG, it will pass it to gcc, which won't understand an ld-specific flag. So, we have to wrap an ld-specific flag twice: the first level will be unwrapped by NAG, the second one - by gcc.
I was going to start working on implementing something like this. I've stumbled upon this issue on a Cray machine. BTW, the colleagues seem to have implemented something addressing this problem: https://easybuild.readthedocs.io/en/latest/RPATH-support.html?highlight=disable-dtags#implementation |
Can you check if this PR solves your issue?
I took a look at that while preparing this PR. It seems to me we are doing the same things conceptually, except that we are also taking care of the reordering of flags as explained here: Lines 279 to 295 in b39ff56
To have an easy reference, Easybuild's code is here. |
b39ff56
to
3a10b6d
Compare
@gartung If you are interested, this branch should permit you to choose between RPATH and RUNPATH. |
@alalazo Thanks for pointing this out. I prefer to use RPATH myself but others in HEP community have expressed interest in using RUNPATH. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alalazo: some general comments -- I think the code could be cleaned up a little if it didn't use 2 variables. I also think fixing NAG (if we're missing RPATHs for it) would be good.
@skosukhin: do you know how frequently people need to do dynamic links with NAG? I'm curious how much RPATHs are used with NAG.
etc/spack/defaults/config.yaml
Outdated
# https://gms.tf/ld_library_path-considered-harmful.html | ||
# | ||
# for more information on the subject. | ||
rpath: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not make this the first entry in the default config.yaml
, as I suspect it's not the first thing most users want to look at. The comment is a bit obscure -- how about:
# Control whether Spack embeds RPATH or RUNPATH attributes in ELF binaries
# so that they can find their dependencies. Has no effect on macOS.
#
# There are two options:
#
# 1. rpath: use RPATH: force --disable-new-tags flag to be passed to ld
# 2. runpath: use RUNPATH: force --enable-new-tags flag to be passed to ld
#
# RPATH search paths have higher precedence than LD_LIBRARY_PATH
# and ld.so will search for libraries in transitive RPATHs of
# parent objects.
#
# RUNPATH search paths have lower precedence than LD_LIBRARRY_PATH,
# and ld.so will ONLY search for dependencies in the RUNPATH of
# the loading object.
#
# DO NOT MIX these within the same install tree. See the Spack
# documentation for details.
And make the two possible options rpath
and runpath
(enforced by schema). This at least gives us room to choose other values in the future.
I think the links there, and maybe also https://bugs.launchpad.net/ubuntu/+source/eglibc/+bug/1253638, could be included in the Spack documentation. Maybe some of the text above could be too... the only important parts for hte config file are really the first couple sentences, the options, and the instructions not to mix them (since if you do, RUNPATH breaks everything).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the Spack documentation for details.
Please point to a specific part of the documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it doesn't exist yet, but the idea was that he'd write it :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that the comment should have a more specific notion on where I should look within the documentation. Having to look through all of it (or using the search function) just seems needlessly mean to the user ;)
# opposite stance that Spack does): | ||
# | ||
# http://blog.qt.io/blog/2011/10/28/rpath-and-runpath/ | ||
# https://wiki.debian.org/RpathIssue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole account is from the point of view of a single-prefix, system package manager. Not one that has to maintain relationships between combinatorial sets of modules... so it's a completely different link situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree completely. I put the links there more as reference for future developers to get some info on RPATH vs. RUNPATH rather than to explain Spack's philosophy towards linking. Let me know if you want me to remove them, or just rephrase the comment.
lib/spack/env/cc
Outdated
@@ -51,6 +51,9 @@ parameters=( | |||
SPACK_CXX_RPATH_ARG | |||
SPACK_F77_RPATH_ARG | |||
SPACK_FC_RPATH_ARG | |||
SPACK_DTAGS_TO_ENABLE | |||
SPACK_DTAGS_TO_DISABLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels very specific to me, and it feels kludgy to have two variables... Could we generalize this? it actually seems like we could do it more cleanly by just adding either --enable-new-dtags
/--disable-new-dtags
to SPACK_LDFLAGS
in the build environment, then a single more general option like SPACK_STRIP_LDFLAGS
, which would strip a set of flags from the link line - not just this one. That mechanism would be a similar amount of code, but there are other places where it would be useful -- For example, I've seen users want to do things like strip fPIC for certain compilers on certain platforms, and we could (in future PRs) allow packages, compilers, or platforms to set some of these things to ease porting. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine to generalize the mechanics here. I'll work towards that.
This feels very specific to me, and it feels kludgy to have two variables...
Eheh, I wouldn't say kludgy, but very specific yes. From my point of view we are broadening the scope of this PR the third time, more or less along the lines:
- Make sure Spack sets RPATH as it says it does
- Give the possibility to users to select between RPATH and RUNPATH
- Extend the mechanism by which we strip flags to
ld
to prepare for functionality that could be added later
To achieve point 2. two variables seemed the right thing to have.
For example, I've seen users want to do things like strip fPIC for certain compilers on certain platforms, and we could
I think fpic
shouldn't be part of SPACK_STRIP_LDFLAGS
but maybe SPACK_STRIP_<LANG>FLAGS
. Anyhow: I agree with the overall idea of intercepting flags we don't want to be in the compiler wrapper and strip them.
@tgamblin, I would say that NAG users need dynamic linking as often as users of any other compiler. I wrote a short text on dynamic linking and Spack explaining why: https://gitlab.dkrz.de/m300488/icon-bootstrap#dynamic-linking. What I have not written there is that in order to support static linking properly, we need the method |
@alalazo if I understand correctly, you want to add -Wl, prefixes to the linking flags assuming they are specified in the form understandable by ld. What is the plan on the compiler linking flags like -pthread, -openmp (and probably other) that need to be passed in the ccld mode but without the prefix? And what prefix will be added when I have a mixed toolchain like NAG+gcc, which require different prefixes (and different flags)? |
That's how I interpreted the request in #9168 (comment). The mechanism should work only for the flags that are injected in the compiler wrapper.
Can't they be injected with
This is a case I effectively didn't consider. If we want to support that we probably need to split |
@alalazo I think we should start with making it possible to override every property of the And a small remark. We can replace this: spack/lib/spack/spack/compiler.py Lines 129 to 136 in 0f053e0
With this (that's how, for example, libtool does this): @property
def fc_rpath_arg(self):
return self.linker_arg + '-rpath ' + self.linker_arg
@property
def linker_arg(self):
"""Flag that need to be used to pass an argument to the linker."""
return '-Wl,' For NAG, for example, that would result in prepending: |
@skosukhin @tgamblin To summarize, I see many sensible needs being discussed here:
As @neilflood confirms that this PR already achieves 1. (and closes the 3 issues in the description) I'd propose the following:
@tgamblin Would that be fine with you, or do you prefer to work all three issues here? What I would like to avoid is having a PR that stays idle for a long time and becomes bigger and bigger... |
0f053e0
to
04fbf82
Compare
04fbf82
to
a2a5c6d
Compare
This PR also lacks documentation. As soon as you consider the design of the feature complete I'll start working on adding that. |
@alalazo: on the hash issue, I think this should probably not have to affect the hash. I say that because we should be able to decide at binary install time whether we want to relocate using |
@tgamblin I wasn't aware that you could switch from one to the other when relocating binaries. I'll try that out and report here. |
f7d597f
to
f354176
Compare
As noted by Todd, ccld needs a prefix (e.g. `-Wl,`) before the linker argument, while ld does not. This prefix changes in principle depending on the compiler. Using (or not) `--disable-new-dtags` is now OS dependent.
If that flag is present in the options passed to the compiler or the linker the wrapper will strip it explicitly.
A new option as been added to config.yaml to select between RPATH and RUNPATH. The default configuration file contains links that point to relevant discussions on the topic.
The option is now named 'shared_linking' and can take two options: 'rpath' and 'runpath'.
f354176
to
f89a951
Compare
d00ffee
to
5a4c6d4
Compare
5a4c6d4
to
aa27fad
Compare
Add a new entry in `config.yaml`: config: shared_linking: 'rpath' If this variable is set to `rpath` (the default) Spack will set RPATH in ELF binaries. If set to `runpath` it will set RUNPATH. Details: * Spack cc wrapper explicitly adds `--disable-new-dtags` when linking * cc wrapper also strips `--enable-new-dtags` from the compile line when disabling (and vice versa) * We specifically do *not* add any dtags flags on macOS, which uses Mach-O binaries, not ELF, so there's no RUNPATH)
fixes #9160
fixes #909
fixes #639
This PR adds a new entry in
config.yaml
:If this variable is set to
rpath
(the default) Spack will set RPATH in ELF binaries. If set torunpath
it will set RUNPATH.