This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Replace SymbolTable::forEachSymbol with iterator_range symbols()
ClosedPublic

Authored by MaskRay on Nov 20 2019, 12:42 PM.

Details

Summary

D62381 introduced forEachSymbol(). It seems that many call sites cannot
be parallelized because the body shared some states. Replace
forEachSymbol with iterator_range<filter_iterator<...>> symbols() to
simplify code and improve debuggability (std::function calls take some
frames).

It also allows us to use early return to simplify code added in D69650.

Event Timeline

MaskRay created this revision.Nov 20 2019, 12:42 PM
ruiu added a comment.Nov 20 2019, 5:56 PM

This is my personal taste, but I feel like the former code is simpler, though it doesn't look elegant. The new code looks a bit too smart to me. I also have a little concern to define start and end to SymbolTable class because it is not very clear what are iterated. SymbolTable contains not only symbols but other members.

This is my personal taste, but I feel like the former code is simpler, though it doesn't look elegant. The new code looks a bit too smart to me. I also have a little concern to define start and end to SymbolTable class because it is not very clear what are iterated. SymbolTable contains not only symbols but other members.

Would it alleviate your concerns if it were more like:

for (Symbol *sym : symtab->symbols())

?

ruiu added a comment.Nov 20 2019, 7:23 PM

This is my personal taste, but I feel like the former code is simpler, though it doesn't look elegant. The new code looks a bit too smart to me. I also have a little concern to define start and end to SymbolTable class because it is not very clear what are iterated. SymbolTable contains not only symbols but other members.

Would it alleviate your concerns if it were more like:

for (Symbol *sym : symtab->symbols())

?

Yes, that would address my second concern.

This is my personal taste, but I feel like the former code is simpler, though it doesn't look elegant. The new code looks a bit too smart to me. I also have a little concern to define start and end to SymbolTable class because it is not very clear what are iterated. SymbolTable contains not only symbols but other members.

Would it alleviate your concerns if it were more like:

for (Symbol *sym : symtab->symbols())

?

symbols() was my original design, but I felt that SymbolTable::{begin,end} is sufficient and easier. This class is called SymbolTable, a collection of symbols. It is intuitive to get a symbol when iterating it. I can't think of a second way to iterate it :) This is similar to tools/llvm-objcopy/ELF/Object.h:SectionTableRef, which has begin/end as its member functions and allows iterating itself. I am open to a change to symbols(), but I wanted to express my preference for begin()/end().

Personally I am in favor of doing this change. The code became much simpler. A little unusual smartness (for LLD code)
added to SymbolTable is really short and it doesn't take much time to understand for any first-time reader what it does
(like it might happen sometimes when much smarter iterators logic is used).
Also, this part is not expected to be modified anymore probably, so hopefully we'll just forget about it and enjoy benefits in the rest of the code.

lld/ELF/Relocations.cpp
806

You need bracers for the whole for loop too.

I'm comfortable with defining a filter iterator, I think the trade off of increased complexity vs simple interface is worth it. I can see some benefit in `for (Symbol *sym : symtab->symbols())` as it is not clear from sym : symtab that PlaceHolders are being skipped.

MaskRay updated this revision to Diff 230562.Nov 21 2019, 4:22 PM
MaskRay retitled this revision from [ELF] Replace SymbolTable::forEachSymbol with filter_iterator begin()/end() to [ELF] Replace SymbolTable::forEachSymbol with iterator_range symbols().
MaskRay edited the summary of this revision. (Show Details)

I can see some benefit in for (Symbol *sym : symtab->symbols()) as it is not clear from sym : symtab that PlaceHolder's are being skipped.

begin()/end() -> symbols()

grimar accepted this revision.Nov 22 2019, 12:30 AM

This LGTM but please wait for others too.

This revision is now accepted and ready to land.Nov 22 2019, 12:30 AM

I'm happy with this too.

I also think this looks much more readable.

@ruiu Are you ok with this change?

This revision was automatically updated to reflect the committed changes.
ruiu added a comment.Nov 26 2019, 9:13 PM

I still feel that I prefer the previous code, but that's not a strong preference, so I'm fine with this.