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

depfixer: install_rpath gets split into multiple with each having one character(!) #13355

Closed
pbrzez opened this issue Jun 26, 2024 · 0 comments · Fixed by #13357
Closed

depfixer: install_rpath gets split into multiple with each having one character(!) #13355

pbrzez opened this issue Jun 26, 2024 · 0 comments · Fixed by #13357
Labels
OS:macos Issues specific to Apple Operating Systems like MacOS and iOS regression
Milestone

Comments

@pbrzez
Copy link
Contributor

pbrzez commented Jun 26, 2024

I'm building a library and setting an install_rpath on it, like so: install_rpath : '/usr/lib/swift'.

Expected behaviour
Before 1.4.1, this would result in the rpath getting correctly added on install, as follows (otool -l library.dylib):

Load command 54
          cmd LC_RPATH
      cmdsize 32
         path /usr/lib/swift (offset 12)

Current behaviour
However, with 1.4.1 onwards (testing on master), this happens:

Load command 69
          cmd LC_RPATH
      cmdsize 16
         path / (offset 12)
Load command 70
          cmd LC_RPATH
      cmdsize 16
         path u (offset 12)
Load command 71
          cmd LC_RPATH
      cmdsize 16
         path s (offset 12)
Load command 72
          cmd LC_RPATH
      cmdsize 16
         path r (offset 12)
Load command 73
          cmd LC_RPATH
      cmdsize 16
         path l (offset 12)
Load command 74
          cmd LC_RPATH
      cmdsize 16
         path i (offset 12)
Load command 75
          cmd LC_RPATH
      cmdsize 16
         path b (offset 12)
Load command 76
          cmd LC_RPATH
      cmdsize 16
         path w (offset 12)
Load command 77
          cmd LC_RPATH
      cmdsize 16
         path f (offset 12)
Load command 78
          cmd LC_RPATH
      cmdsize 16
         path t (offset 12)

Which is, obviously, quite not right :)
Not sure if that makes a difference, but I also manually add that rpath via -Wl,-rpath at link time. No matter what though, this shouldn't happen.

The faulty code was added here: https://github.com/mesonbuild/meson/pull/13012/files#diff-d708bb4ba1cd44eef4be4fa7a95a66e78e29816178905b4a95b5276eaa046431R409

new_rpaths: OrderedSet[str] = OrderedSet()
if new_rpath:
new_rpaths.update(new_rpath)

new_rpath is a str, so this splits it into separate entries per character I think...

system parameters
macOS 14.5, Meson 1.4.1 onwards

This is quite breaking, and would be nice to get this fixed for 1.5.0 if possible.

As a side note, the linked PR is also causing another set of differences in my builds (a lot of RPATHs being left over after installing, compared to earlier on, and I'm sure they are supposed to be removed even after that PR) but I'll make a separate issue once I make sure it's definitely not correct.

@eli-schwartz eli-schwartz added this to the 1.4.2 milestone Jun 26, 2024
bruchar1 added a commit to bruchar1/meson that referenced this issue Jun 27, 2024
eli-schwartz added a commit that referenced this issue Jul 10, 2024
Fixes regression in commit 78e9009.

new_rpath is only set when install_rpath appears in meson.build.

Before this commit, we treated new_rpath as a single string to pass to
-add_rpath. This worked as long as new_rpath had a single rpath in it,
though for ELF we explicitly supported multiple rpaths separated with
":" (as binutils ld is quite happy with that too).

install_name_tool does not support paths with colons in it:
```
21:12 <awilfox> Load command 19          cmd LC_RPATH      cmdsize 40         path /bar:/baz:/eli:/ldap (offset 12)
21:12 <awilfox> Load command 20          cmd LC_RPATH      cmdsize 24         path /foo (offset 12)
21:14 <awilfox> so the result is: do not use colons
```

After commit 78e9009, we simply ended
up with one load command for LC_RPATH per char in new_rpath, which was
wrong in every possible case.

What we actually need to do is pass every distinct rpath as a separate
`-add_rpath` argument, so we split it on the colons instead. We do the
same splitting to ensure proper diff'ability for ELF anyways...

Fixes #13355
(cherry picked from commit 7b43a2e)
@thesamesam thesamesam added regression OS:macos Issues specific to Apple Operating Systems like MacOS and iOS labels Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS:macos Issues specific to Apple Operating Systems like MacOS and iOS regression
Projects
None yet
3 participants