Page MenuHomePhabricator

Use SymbolTable::insert() to implement --trace.
ClosedPublic

Authored by ruiu on May 24 2019, 5:42 AM.

Details

Summary

If a traced symbol is not inserted at all, a placeholder symbol
remains in the symbol table, but that's OK. For the parallel symbol
resolution, I'm planning to insert lots of placeholder symbols which
will remain as placeholder symbols after symbol resolution.

Diff Detail

Repository
rL LLVM

Event Timeline

ruiu created this revision.May 24 2019, 5:42 AM
grimar added inline comments.May 24 2019, 6:08 AM
lld/ELF/Driver.cpp
1403 ↗(On Diff #201208)

Should we filter out placeholder symbols returned by getSymbols somehow instead?

lld/ELF/Writer.cpp
1655 ↗(On Diff #201208)

Combine into single if?

grimar added inline comments.May 24 2019, 6:23 AM
lld/ELF/Driver.cpp
1403 ↗(On Diff #201208)

I am thinking about using llvm iterators for example. Just do not sure that spreading !S->isPlaceholder() around looks fine.

MaskRay added inline comments.May 24 2019, 8:20 AM
lld/ELF/Driver.cpp
1403 ↗(On Diff #201208)

Something like getNonPlaceholderSymbols() based on llvm::make_filter_range?

ruiu updated this revision to Diff 201476.May 26 2019, 11:08 PM
  • replace getSymbols with forEachSymbol
ruiu marked an inline comment as done.May 26 2019, 11:30 PM
ruiu added inline comments.
lld/ELF/Driver.cpp
1403 ↗(On Diff #201208)

make_filter_range should work, but if we use the function, getSymbols's function signature would be complicated, so I decided to use a lambda instead. How does this look?

grimar accepted this revision.May 27 2019, 2:07 AM

Looks great! LGTM.

This revision is now accepted and ready to land.May 27 2019, 2:07 AM
grimar added inline comments.May 27 2019, 3:34 AM
lld/ELF/SymbolTable.h
38 ↗(On Diff #201476)

llvm::function_ref should probably be a bit better, btw.

ruiu marked an inline comment as done.May 27 2019, 3:41 AM
ruiu added inline comments.
lld/ELF/SymbolTable.h
38 ↗(On Diff #201476)

I didn't take a look at the assembly, but isn't this something that a compiler can optimize and erases std::function? Since this is an inline function, a compiler knows exactly how a given lambda is used.

grimar added inline comments.May 27 2019, 3:47 AM
lld/ELF/SymbolTable.h
38 ↗(On Diff #201476)

I do not know the answer.

MaskRay added inline comments.May 27 2019, 6:34 PM
lld/ELF/SymbolTable.h
38 ↗(On Diff #201476)

function_ref should be better, but I don't know if compilers can optimize out the heap allocation used for the lambda.

I still want to understand how complicated the signature of llvm::make_filter_range is. Let me check

MaskRay added inline comments.May 27 2019, 6:47 PM
lld/ELF/SymbolTable.h
38 ↗(On Diff #201476)
struct FilterOutPlaceholder {
  bool operator()(Symbol *S) const {
    return !S->isPlaceholder();
  }
};

static iterator_range<filter_iterator<std::vector<Symbol *>::iterator, FilterOutPlaceholder>> symbols() {
  return make_filter_range(Symtab->SymVector, FilterOutPlaceholder());
}

The benefit is that the backtrace will be simpler in a debugger.

ruiu marked an inline comment as done.May 27 2019, 8:55 PM
ruiu added inline comments.
lld/ELF/SymbolTable.h
38 ↗(On Diff #201476)

We can do that too, but it seems a bit more complicated than the lambda.

I'm also making a change to split SymVector and SymMap into multiple shards for parallel name resolution. With that, the iterator would have more complicated type, as you need to concatenate multiple iterators and then filter out some members after that.

ruiu updated this revision to Diff 201608.May 27 2019, 9:32 PM
  • Change the callback object type from std:function to llvm::function_ref. Looks like Clang can eliminate a given lambda and inline it if we take it as a llvm::function_ref.
MaskRay accepted this revision.May 27 2019, 10:02 PM
MaskRay added inline comments.
lld/ELF/SymbolTable.h
38 ↗(On Diff #201476)

Since you plan to split SymVector, a linear for look no longer works in call sites, this looks good to me, otherwise I would still think the for (auto xxx : SymTable->symbols()) ... is better than Symtab->forEachSymbol([](Symbol *Sym) { ... });

This revision was automatically updated to reflect the committed changes.