Skip to content

compiler wrapper: parse Wl and Xlinker properly#35912

Merged
alalazo merged 2 commits intospack:developfrom
haampie:fix/compiler-wrapper-Wl-parsing
Mar 8, 2023
Merged

compiler wrapper: parse Wl and Xlinker properly#35912
alalazo merged 2 commits intospack:developfrom
haampie:fix/compiler-wrapper-Wl-parsing

Conversation

@haampie
Copy link
Copy Markdown
Member

@haampie haampie commented Mar 7, 2023 •
edited
Loading

Two fixes:

  1. -Wl,a,b,c,d is a comma separated list of linker arguments, we
    incorrectly assume flags or key/value pairs, which runs into issues with for
    example -Wl,--enable-new-dtags,-rpath,/x
  2. -Xlinker,xxx is not a thing, so it shouldn't be parsed.

A third issue not fixed here is that ld -R <path> is an alternative to
-rpath when <path> is a directory. That's rather odd behavior... in
principle we can keep track of -R and conditionally add with [ -d "$rp" ]
but imho thats Too Much for a rarely used flag.

Does this fix your issue @giordano?

@haampie
compiler wrapper: parse Wl and Xlinker properly
425f17d 
Two fixes:

1. `-Wl,a,b,c,d` is a comma separated list of linker arguments, we
   incorrectly assume key/value pairs, which runs into issues with for
   example `-Wl,--enable-new-dtags,-rpath,/x`
2. `-Xlinker,xxx` is not a think, so it shouldn't be parsed.
@spackbot-app spackbot-app bot added core PR affects Spack core functionality tests General test capability(ies) labels Mar 7, 2023
@haampie haampie requested a review from tgamblin March 7, 2023 16:24
@haampie haampie added this to the v0.19.2 milestone Mar 7, 2023
@haampie haampie added the bugfix Something wasn't working, here's a fix label Mar 7, 2023
@giordano
Copy link
Copy Markdown
Member

giordano commented Mar 7, 2023

Yes! Now I can load the python bindings of openmm:

[cceamgi@login13 compute-node]$ eval $(spack -e . load --sh openmm)
[cceamgi@login13 compute-node]$ python
Python 3.10.8 (main, Mar  1 2023, 19:48:02) [GCC 10.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from openmm import app
>>>

For future reference, the problem I had was

[cceamgi@login12 compute-node]$ eval $(spack -e . load --sh openmm)
[cceamgi@login12 compute-node]$ python 
Python 3.10.8 (main, Mar  1 2023, 19:48:02) [GCC 10.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from openmm import app
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/lustre/scratch/scratch/cceamgi/repo/excalibur-tests/spack/myriad/compute-node/opt/linux-rhel7-skylake_avx512/gcc-10.2.0/openmm-7.7.0-bn7gwyx6reh2rxqrnxdk72dp7bsyjxoj/lib/python3.10/site-packages/simtk/__init__.py", line 1, in <module>
    import openmm
  File "/lustre/scratch/scratch/cceamgi/repo/excalibur-tests/spack/myriad/compute-node/opt/linux-rhel7-skylake_avx512/gcc-10.2.0/openmm-7.7.0-bn7gwyx6reh2rxqrnxdk72dp7bsyjxoj/lib/python3.10/site-packages/openmm/__init__.py", line 19, in <module>
    from openmm.openmm import *
  File "/lustre/scratch/scratch/cceamgi/repo/excalibur-tests/spack/myriad/compute-node/opt/linux-rhel7-skylake_avx512/gcc-10.2.0/openmm-7.7.0-bn7gwyx6reh2rxqrnxdk72dp7bsyjxoj/lib/python3.10/site-packages/openmm/openmm.py", line 10, in <module>
    from . import _openmm
ImportError: /shared/ucl/apps/gcc/4.9.2/lib64/libstdc++.so.6: version `GLIBCXX_3.4.21' not found (required by /lustre/scratch/scratch/cceamgi/repo/excalibur-tests/spack/myriad/compute-node/opt/linux-rhel7-skylake_avx512/gcc-10.2.0/openmm-7.7.0-bn7gwyx6reh2rxqrnxdk72dp7bsyjxoj/lib/python3.10/site-packages/openmm/_openmm.cpython-310-x86_64-linux-gnu.so)
>>>

because the python bindings _openmm.cpython-310-x86_64-linux-gnu.so is compiled with -Wl,--enable-new-dtags,-R,/some/path, which before this PR wasn't handled by Spack, and so the rpath was set incorrectly.

Thanks! 🚀

@haampie
Copy link
Copy Markdown
Member Author

haampie commented Mar 7, 2023 •
edited
Loading

Regarding -Xlinker I think that's still wrong. It doesn't support all combinations of:

  • -Xlinker -rpath=/x
  • -Xlinker --rpath=/x
  • -Xlinker -rpath -Xlinker /x
  • -Xlinker --rpath -Xlinker /x

And I expect (but didn't test) that -Xlinker -rpath f.c g.c h.c -Xlinker /x is supported in principle, which we don't handle either.

I'll leave that for another PR...

@haampie
Copy link
Copy Markdown
Member Author

haampie commented Mar 7, 2023

@spackbot run pipeline

@spackbot-app
Copy link
Copy Markdown

spackbot-app bot commented Mar 7, 2023

I've started that pipeline for you!

@alalazo alalazo self-assigned this Mar 8, 2023
@alalazo alalazo merged commit c37d6f9 into spack:develop Mar 8, 2023
@haampie haampie deleted the fix/compiler-wrapper-Wl-parsing branch March 8, 2023 08:06
haampie added a commit that referenced this pull request Mar 8, 2023
@haampie
compiler wrapper: parse Wl and Xlinker properly (#35912)
7cb5664 
Two fixes:

1. `-Wl,a,b,c,d` is a comma separated list of linker arguments, we
   incorrectly assume key/value pairs, which runs into issues with for
   example `-Wl,--enable-new-dtags,-rpath,/x`
2. `-Xlinker,xxx` is not a think, so it shouldn't be parsed.
@haampie haampie mentioned this pull request Mar 8, 2023
Merged
@giordano giordano mentioned this pull request Mar 13, 2023
Merged
@haampie haampie mentioned this pull request Mar 31, 2023
Merged
13 tasks
haampie added a commit that referenced this pull request Apr 3, 2023
@haampie
compiler wrapper: parse Wl and Xlinker properly (#35912)
6b5f255 
Two fixes:

1. `-Wl,a,b,c,d` is a comma separated list of linker arguments, we
   incorrectly assume key/value pairs, which runs into issues with for
   example `-Wl,--enable-new-dtags,-rpath,/x`
2. `-Xlinker,xxx` is not a think, so it shouldn't be parsed.
alalazo pushed a commit that referenced this pull request Apr 3, 2023
@haampie @alalazo
compiler wrapper: parse Wl and Xlinker properly (#35912)
16052f9 
Two fixes:

1. `-Wl,a,b,c,d` is a comma separated list of linker arguments, we
   incorrectly assume key/value pairs, which runs into issues with for
   example `-Wl,--enable-new-dtags,-rpath,/x`
2. `-Xlinker,xxx` is not a think, so it shouldn't be parsed.
jmcarcell pushed a commit to key4hep/spack that referenced this pull request Apr 13, 2023
@haampie @jmcarcell
compiler wrapper: parse Wl and Xlinker properly (spack#35912)
40ce201 
Two fixes:

1. `-Wl,a,b,c,d` is a comma separated list of linker arguments, we
   incorrectly assume key/value pairs, which runs into issues with for
   example `-Wl,--enable-new-dtags,-rpath,/x`
2. `-Xlinker,xxx` is not a think, so it shouldn't be parsed.
@hpcnpatel hpcnpatel mentioned this pull request Apr 25, 2023
Closed
3 tasks
@haampie haampie mentioned this pull request Apr 26, 2023
Merged
vjranagit pushed a commit to vjranagit/spack that referenced this pull request Jan 18, 2026
@haampie @alalazo
compiler wrapper: parse Wl and Xlinker properly (spack#35912)
62d12e0 
Two fixes:

1. `-Wl,a,b,c,d` is a comma separated list of linker arguments, we
   incorrectly assume key/value pairs, which runs into issues with for
   example `-Wl,--enable-new-dtags,-rpath,/x`
2. `-Xlinker,xxx` is not a think, so it shouldn't be parsed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalazo alalazo alalazo approved these changes

@tgamblin tgamblin Awaiting requested review from tgamblin

Assignees

@alalazo alalazo

Labels

bugfix Something wasn't working, here's a fix core PR affects Spack core functionality tests General test capability(ies)

Projects

None yet

Milestone

v0.19.2

Development

Successfully merging this pull request may close these issues.

3 participants

@haampie @giordano @alalazo