This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Uniquify --wrap list.
ClosedPublic

Authored by MaskRay on Jun 18 2018, 2:15 PM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jun 18 2018, 2:15 PM
echristo accepted this revision.Jun 18 2018, 3:33 PM

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.

This revision is now accepted and ready to land.Jun 18 2018, 3:33 PM
MaskRay updated this revision to Diff 151806.Jun 18 2018, 3:36 PM
MaskRay marked an inline comment as done.

Update test

This revision was automatically updated to reflect the committed changes.
grimar added a subscriber: grimar.Jun 19 2018, 1:32 AM
grimar added inline comments.
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?

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 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 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"));
}

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).

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!