toolchains_llvm

toolchains_llvm copied to clipboard

H/t to @vinistock for finding this issue.

The below is mostly copied from #88; the issue is essentially that we're parsing -install_path @executable_path/... and similar flags as param files, incorrectly.

---

That snippet was added to support parameter files.

The macOS cc wrapper script inspects the full command line in order to remap libraries that are being linked against to their fully resolved paths, taking into account the rpaths added to the binary. I don't have first-hand experience with this but this is allegedly because of some oddness having to do with runpaths added to binaries (this has some context; I think it's that the paths are relative to the build dir and not the workspace that's causing the issue but I have no idea what's introducing the -Wl,-rpaths in the first place).

Anyways, for that reason we need to actually read what's in the parameter file. The PR in this repo you linked to was essentially copied from upstream (this commit); in general the logic in the macOS wrapper mostly comes from upstream.

The issue here, of course, is that the @ in -install_name @execution_path/... does not signify a parameter file!

What's peculiar to me is that upstream Bazel seems to fail on this in the exact same way (here's a minimal test case). Perhaps it's simply not common for users to want to generate dylibs with install_paths from Bazel and it hasn't come up? Not sure.

---

Extending the logic in the macOS wrapper to skip processing args starting with @ (like @execution_path/..., @executable_path/..., @load_path/.., @rpath/..., etc.) when the preceeding arg is -install_name or -rpath is the obvious fix, I think.

I have a few concerns though:

• is this genuinely an error with the default Bazel toolchain?
  • edit: it appears so: bazelbuild/bazel#13044
• are -install_name and -rpath the only args that accept @ macro form args? might be worth checking the clang arg parsing source to be sure
  • edit: it's probably better to just check that the file exists instead of maintaining a list. FWIW clang and the linker don't seem to actually even have a list of @ macros that are acceptable; passing in @some_random_thing will compile successfully. grepping through dyld source and peering through the dyld man pages does indicate that it's really just @rpath, @executable_path, and @loader_path though. (@execution_path seems to be just a typo that a few people independently made over the years)
• what kind of path remapping should we be doing for -install_name and -rpath?
  • I think the answer is "none" for -install_name but what about -rpath? Don't we have the same issues as with -Wl,-rpath? -rpath certainly does seem to just expand out into -rpath to the linker, experimentally. Are we just banking on users always using the not-macOS-specific -Wl form?
    • edit: going to add -rpath @loader_path/... to $RPATHS too; this misses -Xlinker -rpath -Xlinker @loader_path/... but I think that's okay for now.
• why does the logic in the wrapper only check for @loader_path?
  • edit: not really sure but assuming this is the path of the directory the dylib being loaded was found at at link time it makes sense that this doesn't match up with the runtime search path, for Bazel. if you don't think about it too hard.

For your reference, the flag is added by Ruby's configure script. As instructed by the README, running autogen.sh creates the configure script, which in turn adds the install_name flag.

Although the configure script itself doesn't exist in the repository (since it's autogenerated), we can see the relevant lines in configure.ac. Here is where it sets the install_name and a few lines above is where the base path is set.

Sorbet's custom build does the same process, integrating it with Bazel. Here is where configure is invoked and install_name is set and the build fails immediately after when compiling (because cc_wrapper will try to read the path).

In respect to your concerns, I'll need to do more research to be of more help. But I can confirm that changing the cc_wrapper to not do any special processing of arguments beginning with @ makes the build succeed. For example, swapping the loop that parses arguments for this does the trick

for i in "$@"; do
    parse_option "$i"
done

In respect to your concerns, I'll need to do more research to be of more help.

Whoops my bad, I wasn't expecting you to try to find the answers to those questions; just thinking out loud.

Sorbet's custom build does the same process, integrating it with Bazel. Here is where configure is invoked and install_name is set and the build fails immediately after when compiling (because cc_wrapper will try to read the path).

Got it, thanks.

Note I believe this case should be fixed once https://github.com/bazelbuild/bazel/pull/13148 lands

Note I believe this case should be fixed once bazelbuild/bazel#13148 lands

Thank you. And thanks for bumping that PR/writing bazelbuild/bazel#14650.

I think the referenced bazel fixes are in bazel 6.0.0 but I'm still seeing failures when using @rrbutani's test case from the gist. Tried on 6.0.0 and 7.0.0-pre.20221212.2.

And I stumbled upon this because I saw failures in a real part of my build, not that test case.

Any idea what's needed?

This may be resolved now with the new wrapper scripts. Can someone please try it and close this issue?

Closing now, but can reopen if someone says something.