Page MenuHomePhabricator

Simplify SymbolTable::add{Defined,Undefined,...} functions.

Authored by ruiu on May 13 2019, 5:59 AM.



SymbolTable's add-family functions have lots of parameters because
when they have to create a new symbol, they forward given arguments
to Symbol's constructors. Therefore, the functions take at least as
many arguments as their corresponding constructors.

This patch simplifies the add-family functions. Now, the functions
take a symbol instead of arguments to construct a symbol. If there's
no existing symbol, a given symbol is memcpy'ed to the symbol table.
Otherwise, the functions attempt to merge the existing and a given
new symbol.

I also eliminated CanOmitFromDynSym parameter, so that the functions
take really one argument.

Symbol classes are trivially constructible, so looks like constructing
them to pass to add-family functions is as cheap as passing a lot of
arguments to the functions. A quick benchmark showed that this patch
seems performance-neutral.

This is a preparation for

Diff Detail


Event Timeline

ruiu created this revision.May 13 2019, 5:59 AM
grimar added inline comments.May 13 2019, 7:33 AM
1379 ↗(On Diff #199259)

What was confusing for me when I saw this in your patch is that Sym is a pointer.
I.e. I was wondering can addUndefined store it somewhere inside or not. Should it be a reference may be?

And then you should be able to:

return Symtab->addUndefined<ELFT>({nullptr, Name, STB_GLOBAL, STV_DEFAULT, 0)});
MaskRay added inline comments.May 13 2019, 7:53 AM
368 ↗(On Diff #199259)

Agree with George. T *New -> const T &New

MaskRay added inline comments.May 13 2019, 8:07 AM
384 ↗(On Diff #199259)

Sym->isLazy() can be deleted because an isLazy() symbol is STT_NOTYPE.

MaskRay added inline comments.May 14 2019, 11:58 PM
121 ↗(On Diff #199259)

It seems after every insert() call, if the IsNew branch is taken, these member variables are initialized then get overridden by replaceSymbol(Old, New). Is there some way to prevent repeated assignments?

ruiu marked 4 inline comments as done.May 15 2019, 5:49 AM

This is the same as the previous patch. If there's no big issue in this patch, I'd like to commit after making changes as you guys pointed out.

1379 ↗(On Diff #199259)

I could indeed make it a const reference. That's perhaps a matter of personal taste, but since I don't feel strongly about . that, I'll change the parameter type as you suggested.

121 ↗(On Diff #199259)

Good point, and I tried that before submitting this patch for review. It turned out that doing that isn't as easy as it seems. There may be some good way to do that, though. I'd want to keep it as-is for now.

368 ↗(On Diff #199259)

Will do.

384 ↗(On Diff #199259)

We actually need this because if we delete Sym->isLazy() and if we have a lazy symbol and a TLS symbol, we'll end up comparing STT_NOTYPE and STT_TLS.

MaskRay accepted this revision.May 15 2019, 6:24 AM

LGTM after the const reference change (void replaceSymbol(Symbol *Sym, const T &New) {).

This revision is now accepted and ready to land.May 15 2019, 6:24 AM
This revision was automatically updated to reflect the committed changes.