[ELF] Uniquify --wrap list.

[MaskRay]

Summary

For --wrap foo --wrap foo, gold wraps the symbol once but LLD would rotate it twice.

[echristo]

One nit, otherwise LGTM.

-eric

test/ELF/wrap.s

7 ↗

(On Diff #151789)

Small nit, I'd probably add a separate test for this rather than changing an existing one (even though it tests the same behavior) so we know in a smaller way if something in particular breaks.

[MaskRay]

Update test

[grimar]

lld/trunk/ELF/Driver.cpp

1310

Honestly saying, it looks weird.

Looking at the code I would say we are trying to wrap the symbols in the particular
alphabetical order, though that is not true. Calling sort it not consistent with how we
handle the command line options in general I believe.

Instead of this, I would suggest to add a small piece into addSymbolWrap:

template <class ELFT> void SymbolTable::addSymbolWrap(StringRef Name) {
....
  // Do not wrap the same symbol twice.
  if (llvm::find_if(WrappedSymbols, [&](const WrappedSymbol &S) {
        return S.Sym == Sym;
      }) != WrappedSymbols.end())
    return;

Rui, what do you think?

[bd1976llvm]

I would propose adding the ability to retrieve a unique filtered list of arguments.

for (StringRef Name : Args.getAllUniqueArgValues(OPT_wrap))
  Symtab->addSymbolWrap<ELFT>(Name);

Relevant for other options: --defsym, --trace-symbol, --undefined etc..

[grimar]

In D48298#1136277, @bd1976llvm wrote:

I would propose adding the ability to retrieve a unique filtered list of arguments.

for (StringRef Name : Args.getAllUniqueArgValues(OPT_wrap))
  Symtab->addSymbolWrap<ELFT>(Name);

Relevant for other options: --defsym, --trace-symbol, --undefined etc..

I am not sure it is useful for other options we have, but it perhaps can be a nice refactoring for this particular case.

For example, I am not sure we should use it for --defsym, as in theory it can have different values: --defsym=foo=1 ... --defsym=foo=2.
It is OK to process all of them one by one like we do now. The same applies for --trace-symbol and --undefined - there is no issue to process them one by one.
I would not overcomplicate the current code for these cases until it is proven to be a useful change.

[MaskRay]

For the record, what bfd and gold do is to record the wrapped symbols. When it later iterates the symbols in some order, it checks whether the symbol has been wrapped, and if true, wrap the symbol.

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob_plain;f=ld/ldmain.c;hb=0d0b0ea29af6abc0790d22f843a3d0cb09424a3a

/* Record a symbol to be wrapped, from the --wrap option.  */

void
add_wrap (const char *name)
{
  if (link_info.wrap_hash == NULL)
    {
      link_info.wrap_hash
	= (struct bfd_hash_table *) xmalloc (sizeof (struct bfd_hash_table));
      if (!bfd_hash_table_init_n (link_info.wrap_hash,
				  bfd_hash_newfunc,
				  sizeof (struct bfd_hash_entry),
				  61))
	einfo (_("%F%P: bfd_hash_table_init failed: %E\n"));
    }

  if (bfd_hash_lookup (link_info.wrap_hash, name, TRUE, TRUE) == NULL)
    einfo (_("%F%P: bfd_hash_lookup failed: %E\n"));
}

[bd1976llvm]

In D48298#1136343, @grimar wrote:

In D48298#1136277, @bd1976llvm wrote:

I would propose adding the ability to retrieve a unique filtered list of arguments.

for (StringRef Name : Args.getAllUniqueArgValues(OPT_wrap))
  Symtab->addSymbolWrap<ELFT>(Name);

Relevant for other options: --defsym, --trace-symbol, --undefined etc..

I am not sure it is useful for other options we have, but it perhaps can be a nice refactoring for this particular case.

For example, I am not sure we should use it for --defsym, as in theory it can have different values: --defsym=foo=1 ... --defsym=foo=2.
It is OK to process all of them one by one like we do now. The same applies for --trace-symbol and --undefined - there is no issue to process them one by one.
I would not overcomplicate the current code for these cases until it is proven to be a useful change.

For --defsym=foo=2 --defsym=foo=1 --defsym=foo=1 --defsym=foo=3 getAllUniqueArgValues() result would be foo=2, foo=1, foo=3. We wouldn't want the result to be sorted.
However, looking at the current use cases of getAllArgValues() it doesn't seem worth adding this at the moment.

Let's go for the approach than you suggested (putting the don't wrap twice logic into addSymbolWrap).

[grimar]

In D48298#1140392, @bd1976llvm wrote:

In D48298#1136343, @grimar wrote:

In D48298#1136277, @bd1976llvm wrote:

I would propose adding the ability to retrieve a unique filtered list of arguments.

for (StringRef Name : Args.getAllUniqueArgValues(OPT_wrap))
  Symtab->addSymbolWrap<ELFT>(Name);

Relevant for other options: --defsym, --trace-symbol, --undefined etc..

I am not sure it is useful for other options we have, but it perhaps can be a nice refactoring for this particular case.

For example, I am not sure we should use it for --defsym, as in theory it can have different values: --defsym=foo=1 ... --defsym=foo=2.
It is OK to process all of them one by one like we do now. The same applies for --trace-symbol and --undefined - there is no issue to process them one by one.
I would not overcomplicate the current code for these cases until it is proven to be a useful change.

For --defsym=foo=2 --defsym=foo=1 --defsym=foo=1 --defsym=foo=3 getAllUniqueArgValues() result would be foo=2, foo=1, foo=3. We wouldn't want the result to be sorted.
However, looking at the current use cases of getAllArgValues() it doesn't seem worth adding this at the moment.

Yeah, it is all fine with foo=2...foo=3 case. There is another one possible though. Currently, we handle --defsym as a virtual linker script command.
(So we perform evaluations one by one)
If we would have --defsym=foo=1 --defsym=foo=foo+1 --defsym=foo+1 then deduplication of --defsym=foo+1 would change the result I believe.
I am not sure how much case is useful/real though (I think not at all), but I would prefer do no changes here than thinking about what might happen :)

Let's go for the approach than you suggested (putting the don't wrap twice logic into addSymbolWrap).

Thanks for comments, I committed rL335337!

lld/trunk/ELF/Driver.cpp

template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
   //
   // For a relocatable output, version scripts don't make sense, and
   // parsing a symbol version string (e.g. dropping "@ver1" from a symbol
   // name "foo@ver1") rather do harm, so we don't call this if -r is given.
   if (!Config->Relocatable)
     Symtab->scanVersionScript();
 
   // Create wrapped symbols for -wrap option.
-  for (auto *Arg : Args.filtered(OPT_wrap))
-    Symtab->addSymbolWrap<ELFT>(Arg->getValue());
+  std::vector<std::string> Wraps = Args.getAllArgValues(OPT_wrap);
+  llvm::sort(Wraps.begin(), Wraps.end());
+  Wraps.erase(std::unique(Wraps.begin(), Wraps.end()), Wraps.end());
+  for (StringRef Name : Wraps)
+    Symtab->addSymbolWrap<ELFT>(Name);
 
   // Do link-time optimization if given files are LLVM bitcode files.
   // This compiles bitcode files into real object files.
   Symtab->addCombinedLTOObject<ELFT>();
   if (errorCount())
     return;
 
   // If -thinlto-index-only is given, we should create only "index

lld/trunk/test/ELF/wrap.s

 // REQUIRES: x86
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t
 // RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/wrap.s -o %t2
 
 // RUN: ld.lld -o %t3 %t %t2 -wrap foo -wrap nosuchsym
 // RUN: llvm-objdump -d -print-imm-hex %t3 | FileCheck %s
 // RUN: ld.lld -o %t3 %t %t2 --wrap foo -wrap=nosuchsym
 // RUN: llvm-objdump -d -print-imm-hex %t3 | FileCheck %s
+// RUN: ld.lld -o %t3 %t %t2 --wrap foo --wrap foo -wrap=nosuchsym
+// RUN: llvm-objdump -d -print-imm-hex %t3 | FileCheck %s
 
 // CHECK: _start:
 // CHECK-NEXT: movl $0x11010, %edx
 // CHECK-NEXT: movl $0x11010, %edx
 // CHECK-NEXT: movl $0x11000, %edx
 
 // RUN: llvm-readobj -t %t3 > %t4.dump
 // RUN: FileCheck --check-prefix=SYM1 %s < %t4.dump