`cc`: ensure that RPATHs passed to linker are unique by tgamblin · Pull Request #46536 · spack/spack

tgamblin · 2024-09-23T06:51:15Z

macOS Sequoia's linker will complain if RPATHs on the CLI are specified more than once. To avoid errors due to this, make cc only append unique RPATHs to the final args list.

This required a few improvements to the logic in cc:

List functions in cc didn't have any way to append only unique elements to a list. Add a contains() shell function that works like our other list functions. Use it to implement an optional "unique" argument to append() and an extend_unique(). Use that to add RPATHs to the args_list.
In the pure ld case, we weren't actually parsing RPATH arguments separately as we do for ccld. Fix this by adding another nested case statement for raw RPATH parsing. There are now 3 places where we deal with -rpath and friends, but I don't see a great way to unify them, as -Wl,, -Xlinker, and raw -rpath arguments are all ever so slightly different.
Fix ordering of assertions to make pytest diffs more intelligible. The meaning of + and - in diffs changed in pytest 6.0 and the "preferred" order for assertions became assert actual == expected instead of the other way around.

This PR also cleans up all the 3-way list inits, assignments, and appends that run through cc, which makes the duplicated structure a little more readable and bearable. Ideally this PR should be left as 2 commits since that stuff isn't really related to the RPATH handling.

tgamblin · 2024-09-23T06:51:29Z

@adamjstewart does this fix #46506 for you?

tgamblin · 2024-09-23T06:54:13Z

This doesn't change the behavior of cc too much but probably merits a full rebuild to make sure we're not inadvertently breaking anything.

adamjstewart · 2024-09-23T09:33:43Z

Rebuilding my entire stack without any -ld_classic workarounds to test this...

adamjstewart · 2024-09-23T13:49:13Z

Okay, I managed to build every single package in my stack (~450) except py-torchvision:

  ld: duplicate LC_RPATH '@loader_path' in '/Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/py-torch-2.4.1-hlpsikrtvukd4ww7jyghncl63ux7f2pl/lib/python3.11/site-packages/torch/lib/libtorch_cpu.dylib'
  clang++: error: linker command failed with exit code 1 (use -v to see invocation)
  error: command '/Users/Adam/spack/lib/spack/env/clang/clang++' failed with exit code 1

Before this PR, there were many more of these issues: pytorch/vision#8653. So it's definitely helping. Any reason that we might still end up with this kind of duplicate for torch?

Builds logs:

tgamblin · 2024-09-23T16:14:13Z

Can't really see the link line in the torch output. Can you run otool -l on torch_cpu.dylib? eg.:

> otool -l $(spack location -i zlib)/lib/libz.dylib|grep -A 2 LC_RPATH
          cmd LC_RPATH
      cmdsize 144
         path /Users/gamblin2/Workspace/src/spack/opt/spack/darwin-sonoma-m1/apple-clang-15.0.0/zlib-1.3-xcoewovwxmf74kd2jyf3gdmw7u6xpgho/lib (offset 12)

And are you able to dig around in CMakeFiles for torch_cpu.dylib and find (I think) the link.txt for it? There should be a directory full of files used to build that target, including the generated Makefiles or ninja files. That whole directory would be useful.

adamjstewart · 2024-09-23T17:58:37Z

> otool -l /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/py-torch-2.4.1-hlpsikrtvukd4ww7jyghncl63ux7f2pl/lib/python3.11/site-packages/torch/lib/libtorch_cpu.dylib | grep -A2 LC_RPATH
          cmd LC_RPATH
      cmdsize 136
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/cpuinfo-2023-11-04-ouyg3anjn4djrytdwgjygcrcocyykr23/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 32
         path @loader_path (offset 12)
--
          cmd LC_RPATH
      cmdsize 136
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/protobuf-3.13.0-lf2th2vux6lq54xhzaho4ofvxp5qhvww/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/zlib-ng-2.2.1-wucxv43f34epmyy5sjrn2yy45flzx62o/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 136
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/py-torch-2.4.1-hlpsikrtvukd4ww7jyghncl63ux7f2pl/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 136
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/py-torch-2.4.1-hlpsikrtvukd4ww7jyghncl63ux7f2pl/lib64 (offset 12)
--
          cmd LC_RPATH
      cmdsize 144
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/sleef-3.6.0_2024-03-20-tgtxw4mhq5ubqf36tergrauecxyfexpo/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 136
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/py-pybind11-2.13.4-kzwmj3go6maqmtyormj4evchhmr5wvrk/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 144
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/pthreadpool-2023-08-29-d6bfgfqytd33imrae2lk52bzjnslork5/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 136
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/openblas-0.3.27-2amqzpce4nsklhp4ox25ewypr3gnkpop/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 136
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/gloo-2023-12-03-dnypmds7obcm3pbrpvvuidupbi2dlxsn/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/libuv-1.48.0-bumrejfvwhazbzafp2lp6p34txdv6jhh/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/python-3.11.9-miamin5zo2vhkrb22ej7xpjqlcjsuugs/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/sqlite-3.46.0-k5zv2uhihslmlfoiu5okz3xlh3lpwmcl/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/openssl-3.3.1-yewqqqz4zyqtly6x3s7isev6hcutxxo4/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 136
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/libxcrypt-4.4.35-xmjwpbwtpxe26u2kcrdrybkgeqlslnjp/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/libffi-3.4.6-qt5u76zwy5qbm6i6m7q2sy5lzisa2jno/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 136
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/gettext-0.22.5-stjeotxb5neptx3pp3tklzhuanaq7jdw/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 136
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/libxml2-2.10.3-pkugvefyak7a43szj3pfcb6ltwx3aejd/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/xz-5.4.6-qihtxelvb2treb6xzd7xy2viodeintmw/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/gdbm-1.23-z4u5qksijcdiuxja3xy6lssmfc6arrf2/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/readline-8.2-p34wq6eaxcu3zqk6gce3edspxjursgdx/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/expat-2.6.3-7zusnj3bc2w7sg65nxm55geajzr7qbg2/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/bzip2-1.0.8-5cutm23eg2ulwkyiu4ujfja6ylilrplo/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/ncurses-6.5-arai6ifyl7drn2uadnvqgdhlzzy7e5ft/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 128
         path /Users/Adam/spack/opt/spack/darwin-sequoia-m2/apple-clang-16.0.0/libiconv-1.17-urwgauhebxjeqk3bimecmvzqyxnt4g2u/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 96
         path /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/System/Library/Frameworks (offset 12)
--
          cmd LC_RPATH
      cmdsize 40
         path /usr/local/gfortran/lib (offset 12)
--
          cmd LC_RPATH
      cmdsize 32
         path @loader_path (offset 12)

Is link.txt something that is created at build time? If you want it for torch, I'll have to rebuild with --keep-stage.

adamjstewart · 2024-09-23T18:42:17Z

I can't find CMakeFiles, maybe I'm blind?

tgamblin · 2024-09-23T19:34:16Z

It would be in the build stage (not the install directory), alongside CMakeCache.txt, though I am not familiar with the ins and outs of torch builds.

tgamblin · 2024-09-23T19:38:44Z

Ok, the duplicate path is @loader_path which is like $ORIGIN on linux (I suppose that was obvious from the error but I wanted to see). I don't think that is Spack's fault -- I think it's from something the py-torch build is doing but I could be wrong. If you can find the actual link line in the build directory I could probably be more sure about it.

adamjstewart · 2024-09-23T22:01:30Z

Tried uploading a zip file of the entire build directory, or even just the build directory for torch_cpu, but they're both too big for GitHub. I couldn't find a link.txt file in the entire tree.

Chrismarsh · 2024-09-24T18:05:54Z

This error also occurs on Sonoma.

Do you think your patch can also fix the issue I reported in this PR?
#46264

Essentially, if the compiler.yaml contains an additional rpath, such as what occurs with the mixed toolchains (e.g., #45479), then this additional rpath can end up as a duplicate, and cause the same error that this PR tries to solve.

tgamblin · 2024-09-25T01:17:02Z

@spackbot rebuild everything

spackbot-app · 2024-09-25T01:17:07Z

I've started that pipeline for you!

tgamblin · 2024-09-25T01:18:38Z

@Chrismarsh

Do you think your patch can also fix the issue I reported in this PR?
#46264

Essentially, if the compiler.yaml contains an additional rpath, such as what occurs with the mixed toolchains (e.g., #45479), then this additional rpath can end up as a duplicate, and cause the same error that this PR tries to solve.

It doesn't solve it as-is, but I can do a follow-on for that. I think somewhere we stopped respecting the add_rpaths variable when we add to the final arg list.

tgamblin · 2024-09-26T20:18:03Z

trying again rebased closer to latest develop.

`cc` divides most paths up into system paths, spack managed paths, and other paths. This gets really repetitive and makes the code hard to read. Simplify the script by adding some functions to do most of the redundant work for us. Signed-off-by: Todd Gamblin <tgamblin@llnl.gov>

macOS Sequoia's linker will complain if RPATHs on the CLI are specified more than once. To avoid errors due to this, make `cc` only append unique RPATHs to the final args list. This required a few improvements to the logic in `cc`: 1. List functions in `cc` didn't have any way to append unique elements to a list. Add a `contains()` shell function that works like our other list functions. Use it to implement an optional `"unique"` argument to `append()` and an `extend_unique()`. Use that to add RPATHs to the `args_list`. 2. In the pure `ld` case, we weren't actually parsing `RPATH` arguments separately as we do for `ccld`. Fix this by adding *another* nested case statement for raw `RPATH` parsing. There are now 3 places where we deal with `-rpath` and friends, but I don't see a great way to unify them, as `-Wl,`, `-Xlinker`, and raw `-rpath` arguments are all ever so slightly different. 3. Fix ordering of assertions to make `pytest` diffs more intelligible. The meaning of `+` and `-` in diffs changed in `pytest` 6.0 and the "preferred" order for assertions became `assert actual == expected` instead of the other way around. Signed-off-by: Todd Gamblin <tgamblin@llnl.gov>

tgamblin · 2024-09-26T20:57:52Z

@spackbot rebuild everything

spackbot-app · 2024-09-26T20:58:24Z

I've started that pipeline for you!

tgamblin · 2024-09-27T00:28:36Z

Ok everything in the pipeline is building successfully with this. @becker33 can you reapprove?

tgamblin · 2024-09-27T00:28:49Z

@adamjstewart I think the issues with py-torch are not related but can continue to investigate

adamjstewart

Fixes almost all the problems I encountered.

tgamblin requested review from adamjstewart and haampie September 23, 2024 06:51

spackbot-app bot added core PR affects Spack core functionality tests General test capability(ies) labels Sep 23, 2024

This was referenced Sep 23, 2024

VTK: fix build with Apple Clang 16 #46463

Closed

seacas: fix build with Apple Clang 16 #46460

Closed

tgamblin requested a review from becker33 September 23, 2024 17:11

tgamblin mentioned this pull request Sep 24, 2024

database: don't call socket.getfqdn() on every write #46554

Merged

1 task

adamjstewart mentioned this pull request Sep 24, 2024

py-torchvision: fix build with Apple Clang 16 #46462

Merged

becker33 previously approved these changes Sep 24, 2024

View reviewed changes

tgamblin force-pushed the cc-unique-rpaths branch from 31b3f5d to 52629c6 Compare September 26, 2024 20:17

tgamblin added 2 commits September 26, 2024 13:28

tgamblin dismissed becker33’s stale review via c052477 September 26, 2024 20:28

tgamblin force-pushed the cc-unique-rpaths branch from 52629c6 to c052477 Compare September 26, 2024 20:28

tgamblin mentioned this pull request Sep 26, 2024

cc: fix RPATH handling with -r on macOS #46598

Open

3 tasks

tgamblin added the linking label Sep 26, 2024

tgamblin requested a review from becker33 September 27, 2024 00:28

adamjstewart approved these changes Sep 27, 2024

View reviewed changes

becker33 approved these changes Sep 27, 2024

View reviewed changes

alalazo assigned becker33 Sep 27, 2024

tgamblin merged commit 2613a14 into develop Sep 27, 2024

tgamblin deleted the cc-unique-rpaths branch September 27, 2024 12:27

adamjstewart mentioned this pull request Sep 27, 2024

Remove -ld_classic from all packages #46608

Merged

haampie mentioned this pull request Oct 2, 2024

Revert "cc: ensure that RPATHs passed to linker are unique" #46712

Merged

tgamblin mentioned this pull request Oct 28, 2024

cc: refactor ld and path list logic #47255

Merged

adamjstewart mentioned this pull request Apr 9, 2025

Installation issue: py-numpy ^openblas %apple-clang@17 #49983

Closed

4 tasks

mathomp4 mentioned this pull request Apr 9, 2025

Issues with XCode 16.3 GEOS-ESM/ESMA_cmake#438

Closed

Conversation

tgamblin commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Sep 23, 2024

Uh oh!

tgamblin commented Sep 23, 2024

Uh oh!

adamjstewart commented Sep 23, 2024

Uh oh!

adamjstewart commented Sep 23, 2024

Uh oh!

tgamblin commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Sep 23, 2024

Uh oh!

adamjstewart commented Sep 23, 2024

Uh oh!

tgamblin commented Sep 23, 2024

Uh oh!

tgamblin commented Sep 23, 2024

Uh oh!

adamjstewart commented Sep 23, 2024

Uh oh!

Chrismarsh commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Sep 25, 2024

Uh oh!

spackbot-app bot commented Sep 25, 2024

Uh oh!

tgamblin commented Sep 25, 2024

Uh oh!

tgamblin commented Sep 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Sep 26, 2024

Uh oh!

spackbot-app bot commented Sep 26, 2024

Uh oh!

tgamblin commented Sep 27, 2024

Uh oh!

tgamblin commented Sep 27, 2024

Uh oh!

adamjstewart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgamblin commented Sep 23, 2024 •

edited

Loading

tgamblin commented Sep 23, 2024 •

edited

Loading

Chrismarsh commented Sep 24, 2024 •

edited

Loading

tgamblin commented Sep 26, 2024 •

edited

Loading