[LLD][ELF] Drop duplicates from rpath

[sberg]

Summary

...to better align with the behavior of at least GNU Binutils ld.bfd (see /* First see whether OPTARG is already in the path. */ ever since https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=252b5132c753830d5fd56823373aed85f2a0db63 "19990502 sourceware import") and ld.gold (see // Eliminate duplicates. since https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=41f542e70b89b9ea2875c438b0c9a60ac46092dd "Add support for -rpath.")

[thieta]

LGTM - there shouldn't be that much use for duplicates here.

Maybe should have a line in the release notes since it changes behavior visible to the user.

[thieta]

(Please don't land this until @MaskRay have had his say).

[sberg]

added to release notes

[MaskRay]

Thanks. This should be fine as it doesn't add many lines. Does this change the behavior in any way?

lld/test/ELF/rpath.test

2 ↗

(On Diff #442288)

Use #

-triple=x86_64

3 ↗

(On Diff #442288)

You may change some -rpath to -rpath

6 ↗

(On Diff #442288)

## for non-directive (CHECK, RUN, etc) comments.

[sberg]

addressed comments on test file

[sberg]

In D129142#3631023, @MaskRay wrote:

Does this change the behavior in any way?

Yes, the non-degenerate test case changes from

Library runpath: [/path1:/path12:/path2:/path2:/path1]

to

Library runpath: [/path1:/path12:/path2]

lld/test/ELF/rpath.test

3 ↗

(On Diff #442288)

You mean, add some variance -rpath=... vs. -rpath ...? But IMO that would just distract from what gets tested here. So I rather updated the test file (and its name) to make it more clear what it is actually testing.

[MaskRay]

In D129142#3631738, @sberg wrote:

In D129142#3631023, @MaskRay wrote:

Does this change the behavior in any way?

Yes, the non-degenerate test case changes from

Library runpath: [/path1:/path12:/path2:/path2:/path1]

to

Library runpath: [/path1:/path12:/path2]

I did not mean this behavior. I mean whether this complexity brings anything beneficial. I don't think the runtime behavior is affected.
Put it another way: is this change necessary.

[sberg]

In D129142#3631741, @MaskRay wrote:

I did not mean this behavior. I mean whether this complexity brings anything beneficial. I don't think the runtime behavior is affected.
Put it another way: is this change necessary.

Ah. The way I came across this is that LibreOffice has some check to make sure that certain of its shared libraries on Linux have a DT_RUNPATH of exactly $ORIGIN. And one of its (bundled external) libraries, built through some libtool invocation, where the LibreOffice build system passes it some LDFLAGS=-Wl,-rpath,$ORIGIN, happened to end up with two such -Wl,-rpath,$ORIGIN on the libtool command line, so when built with lld had a DT_RUNPATH of $ORIGIN:$ORIGIN, so the check for exactly $ORIGIN failed.

So while this shouldn't affect runtime behavior of the resulting object, it was a divergence from GNU behavior that got in the way for me.

[MaskRay]

In D129142#3631759, @sberg wrote:

In D129142#3631741, @MaskRay wrote:

I did not mean this behavior. I mean whether this complexity brings anything beneficial. I don't think the runtime behavior is affected.
Put it another way: is this change necessary.

Ah. The way I came across this is that LibreOffice has some check to make sure that certain of its shared libraries on Linux have a DT_RUNPATH of exactly $ORIGIN. And one of its (bundled external) libraries, built through some libtool invocation, where the LibreOffice build system passes it some LDFLAGS=-Wl,-rpath,$ORIGIN, happened to end up with two such -Wl,-rpath,$ORIGIN on the libtool command line, so when built with lld had a DT_RUNPATH of $ORIGIN:$ORIGIN, so the check for exactly $ORIGIN failed.

So while this shouldn't affect runtime behavior of the resulting object, it was a divergence from GNU behavior that got in the way for me.

OK. Then it seems like a problem in LibreOffice. It passes two -rpath and then has a brittle assumption that -rpath is deduplicated.
lld has always been behaving this way and there are users linking LibreOffice with lld fine, so I am unsure porting this surprising behavior is necessary.
Technically the increased complexity isn't that high (4 lines) but the justification does not seem strong enough.

[MaskRay]

Actually the simple lld behavior has an advantage that it can catch such duplicate -rpath usage, which is probably not intended, but never will be caught with GNU ld or gold.

[MaskRay]

This GNU ld behavior is surprising (deviating from how options are usually handled, e.g. --auxiliary a --auxiliary b) and not matching it does not actually cause a problem to reasonable users.
The mentioned LibreOffice use is a problem in itself.
Actually I'd consider this a bug in GNU ld, not respecting duplicate entries.

[sberg]

In D129142#3631781, @MaskRay wrote:

This GNU ld behavior is surprising (deviating from how options are usually handled, e.g. --auxiliary a --auxiliary b) and not matching it does not actually cause a problem to reasonable users.

But still, both ld.bdf and ld.gold have code to explicitly prune such duplicate -rpath arguments (see the code pointers in the summary). So even though neither of those two linkers appears to give a rationale for that behavior, it nevertheless appears to be a deliberate decision to behave that way. I asked for a rationale now at https://sourceware.org/pipermail/binutils/2022-July/121626.html "Deliberate deduplication of -rpath arguments in ld.bfd and ld.gold?"

The mentioned LibreOffice use is a problem in itself.

(Technically, it's an issue with cURL bundled as part of a LibreOffice build. Witness

$ git clone https://github.com/curl/curl.git
$ cd curl
$ git fetch --tag
$ git checkout curl-7_84_0
$ autoreconf -fi
$ ./configure --without-ssl 'LDFLAGS=-fuse-ld=lld -Wl,-rpath=\$$ORIGIN'
$ make
$ readelf -d lib/.libs/libcurl.so | grep RUNPATH
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN:$ORIGIN]

So faced with the options to either dig into cURL's libtool-based build system or find out why lld behavior apparently deviates from GNU Binutils behavior here, I chose the more pleasant latter option :)

Actually I'd consider this a bug in GNU ld, not respecting duplicate entries.

see above

[sberg]

In D129142#3631968, @sberg wrote:

In D129142#3631781, @MaskRay wrote:

This GNU ld behavior is surprising (deviating from how options are usually handled, e.g. --auxiliary a --auxiliary b) and not matching it does not actually cause a problem to reasonable users.

But still, both ld.bdf and ld.gold have code to explicitly prune such duplicate -rpath arguments (see the code pointers in the summary). So even though neither of those two linkers appears to give a rationale for that behavior, it nevertheless appears to be a deliberate decision to behave that way. I asked for a rationale now at https://sourceware.org/pipermail/binutils/2022-July/121626.html "Deliberate deduplication of -rpath arguments in ld.bfd and ld.gold?"

Quoting the answer at https://sourceware.org/pipermail/binutils/2022-July/121628.html "Re. Deliberate deduplication of -rpath arguments in ld.bfd and ld.gold?":

The rationale would be "avoid waste in the real world". If you don't do that then you will find that when building certain software it will end up with hundreds of copies of things like "RUNPATH /usr/lib/my-strange-module-path/" because its build-system is strange, and blindly collects linker arguments from e.g. pkg-config files, but doesn't remove duplicates itself. As the runtime linker doesn't do duplicate removal either (speed!) it would then end up looking into loading the same non-existing file that same hundred times.
Instead of fixing all these build systems, something simply has to remove the duplicates and the linker is the most sensible choice.

[thieta]

Yeah - I am still in favor of this. There is no reason to have duplicates in the files and a lot of build systems suck, so a simple patch like that seems like a fair trade-off.

[MaskRay]

In D129142#3632403, @sberg wrote:

In D129142#3631968, @sberg wrote:

In D129142#3631781, @MaskRay wrote:

This GNU ld behavior is surprising (deviating from how options are usually handled, e.g. --auxiliary a --auxiliary b) and not matching it does not actually cause a problem to reasonable users.

But still, both ld.bdf and ld.gold have code to explicitly prune such duplicate -rpath arguments (see the code pointers in the summary). So even though neither of those two linkers appears to give a rationale for that behavior, it nevertheless appears to be a deliberate decision to behave that way. I asked for a rationale now at https://sourceware.org/pipermail/binutils/2022-July/121626.html "Deliberate deduplication of -rpath arguments in ld.bfd and ld.gold?"

Quoting the answer at https://sourceware.org/pipermail/binutils/2022-July/121628.html "Re. Deliberate deduplication of -rpath arguments in ld.bfd and ld.gold?":

The rationale would be "avoid waste in the real world". If you don't do that then you will find that when building certain software it will end up with hundreds of copies of things like "RUNPATH /usr/lib/my-strange-module-path/" because its build-system is strange, and blindly collects linker arguments from e.g. pkg-config files, but doesn't remove duplicates itself. As the runtime linker doesn't do duplicate removal either (speed!) it would then end up looking into loading the same non-existing file that same hundred times.
Instead of fixing all these build systems, something simply has to remove the duplicates and the linker is the most sensible choice.

Note that the argument isn't very strong and is more about the behavior may provide a benefit and "since ld does this for so long, we just keep the behavior".
But this is not sufficient to add the inconsistency/complexity to another linker when there is really no behavior difference.
I think losing the strictness is a shame in this particular case as doesn't seem to cause problems in the wild.
LibreOffice likely should fix its build system instead.

[thieta]

clearing my queue

lld/ELF/Driver.cpp

 #include "lld/Common/Filesystem.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Strings.h"
 #include "lld/Common/TargetOptionsCommandFlags.h"
 #include "lld/Common/Version.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/ADT/UniqueVector.h"
 #include "llvm/Config/llvm-config.h"
 #include "llvm/LTO/LTO.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Remarks/HotnessThresholdParser.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compression.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/GlobPattern.h"
if (config->timeTraceEnabled) {
     checkError(timeTraceProfilerWrite(
         args.getLastArgValue(OPT_time_trace_eq).str(), config->outputFile));
     timeTraceProfilerCleanup();
   }
 }
 
 static std::string getRpath(opt::InputArgList &args) {
   std::vector<StringRef> v = args::getStrings(args, OPT_rpath);
-  return llvm::join(v.begin(), v.end(), ":");
+  llvm::UniqueVector<StringRef> u;
+  for (StringRef p : v)
+      u.insert(p);
+  return llvm::join(u.begin(), u.end(), ":");
 }
 
 // Determines what we should do if there are remaining unresolved
 // symbols after the name resolution.
 static void setUnresolvedSymbolPolicy(opt::InputArgList &args) {
   UnresolvedPolicy errorOrWarn = args.hasFlag(OPT_error_unresolved_symbols,
                                               OPT_warn_unresolved_symbols, true)
                                      ? UnresolvedPolicy::ReportError

lld/docs/ReleaseNotes.rst

 ELF Improvements
 ----------------
 
 * ``-z pack-relative-relocs`` is now available to support ``DT_RELR`` for glibc 2.36+.
   (`D120701 <https://reviews.llvm.org/D120701>`_)
 * ``--no-fortran-common`` (pre 12.0.0 behavior) is now the default.
 * ``FORCE_LLD_DIAGNOSTICS_CRASH`` environment variable is now available to force LLD to crash.
   (`D128195 <https://reviews.llvm.org/D128195>`_)
+* Duplicate ``-rpath`` arguments with an identical path are ignored now, matching GNU ld and gold
+  behavior.
+  (`D129142 <https://reviews.llvm.org/D129142>`_)
 
 Breaking changes
 ----------------
 
 * The GNU ld incompatible ``--no-define-common`` has been removed.
 * The obscure ``-dc``/``-dp`` options have been removed.
 * ``-d`` is now ignored.
 * If a prevailing COMDAT group defines STB_WEAK symbol, having a STB_GLOBAL symbol in a non-prevailing group is now rejected with a diagnostic.

lld/test/ELF/duplicated-rpath.test

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64 %s -o %t.o
+
+## Duplicated /path1 and /path2 shall only appear once each:
+# RUN: ld.lld %t.o -rpath=/path1 -rpath=/path12 -rpath=/path2 -rpath=/path2 -rpath=/path1 -shared -o %t
+# RUN: llvm-readobj --dynamic-table %t | FileCheck %s
+# CHECK: Library runpath: [/path1:/path12:/path2]
+
+## Degenerate case where an -rpath argument already contains a colon:
+# RUN: ld.lld %t.o -rpath=/path1 -rpath=/path1:/path2 -rpath=/path2 -shared -o %t
+# RUN: llvm-readobj --dynamic-table %t | FileCheck --check-prefix=DEGENERATE %s
+# DEGENERATE: Library runpath: [/path1:/path1:/path2:/path2]