Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

Differential D33621 Diff 101280 lld/ELF/SymbolTable.cpp

Changeset View

Standalone View

View Options

lld/ELF/SymbolTable.cpp

Show First 20 Lines • Show All 112 Lines • ▼ Show 20 Lines
// to the compiler at once, it can do whole-program optimization. // to the compiler at once, it can do whole-program optimization.
template <class ELFT> void SymbolTable<ELFT>::addCombinedLTOObject() { template <class ELFT> void SymbolTable<ELFT>::addCombinedLTOObject() {
if (BitcodeFiles.empty()) if (BitcodeFiles.empty())
return; return;
// Compile bitcode files and replace bitcode symbols. // Compile bitcode files and replace bitcode symbols.
LTO.reset(new BitcodeCompiler); LTO.reset(new BitcodeCompiler);
for (BitcodeFile *F : BitcodeFiles) for (BitcodeFile *F : BitcodeFiles)
LTO->add(*F); LTO->add(*F);
pccUnsubmitted
Not Done
ReplyInline Actions

If you move the list of renamed symbols to Config you can avoid this lambda.

pcc: If you move the list of renamed symbols to `Config` you can avoid this lambda.
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

OK, will do.

dmikulin: OK, will do.
for (InputFile *File : LTO->compile()) { for (InputFile *File : LTO->compile()) {
ObjectFile<ELFT> *Obj = cast<ObjectFile<ELFT>>(File); ObjectFile<ELFT> *Obj = cast<ObjectFile<ELFT>>(File);
DenseSet<CachedHashStringRef> DummyGroups; DenseSet<CachedHashStringRef> DummyGroups;
Obj->parse(DummyGroups); Obj->parse(DummyGroups);
ObjectFiles.push_back(Obj); ObjectFiles.push_back(Obj);
} }
} }
template <class ELFT> template <class ELFT>
davideUnsubmitted
Done
ReplyInline Actions

Convention says all Variable names should start with capital letter. Also, i is a little vague.

davide: Convention says all Variable names should start with capital letter. Also, `i` is a little…
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

Changed it to RSI (Renamed Symbol Iterator)

dmikulin: Changed it to RSI (Renamed Symbol Iterator)
DefinedRegular *SymbolTable<ELFT>::addAbsolute(StringRef Name, DefinedRegular *SymbolTable<ELFT>::addAbsolute(StringRef Name,
uint8_t Visibility, uint8_t Visibility,
davideUnsubmitted
Not Done
ReplyInline Actions

Do you need an explicit iterator? Can't you use a range for ?

davide: Do you need an explicit iterator? Can't you use a `range for` ?
uint8_t Binding) { uint8_t Binding) {
Symbol *Sym = Symbol *Sym =
addRegular(Name, Visibility, STT_NOTYPE, 0, 0, Binding, nullptr, nullptr); addRegular(Name, Visibility, STT_NOTYPE, 0, 0, Binding, nullptr, nullptr);
return cast<DefinedRegular>(Sym->body()); return cast<DefinedRegular>(Sym->body());
} }
// Add Name as an "ignored" symbol. An ignored symbol is a regular // Add Name as an "ignored" symbol. An ignored symbol is a regular
// linker-synthesized defined symbol, but is only defined if needed. // linker-synthesized defined symbol, but is only defined if needed.
template <class ELFT> template <class ELFT>
DefinedRegular *SymbolTable<ELFT>::addIgnored(StringRef Name, DefinedRegular *SymbolTable<ELFT>::addIgnored(StringRef Name,
uint8_t Visibility) { uint8_t Visibility) {
SymbolBody *S = find(Name); SymbolBody *S = find(Name);
if (!S || S->isInCurrentDSO()) if (!S || S->isInCurrentDSO())
return nullptr; return nullptr;
return addAbsolute(Name, Visibility); return addAbsolute(Name, Visibility);
} }
// Set a flag for --trace-symbol so that we can print out a log message // Set a flag for --trace-symbol so that we can print out a log message
// if a new symbol with the same name is inserted into the symbol table. // if a new symbol with the same name is inserted into the symbol table.
template <class ELFT> void SymbolTable<ELFT>::trace(StringRef Name) { template <class ELFT> void SymbolTable<ELFT>::trace(StringRef Name) {
Symtab.insert({CachedHashStringRef(Name), {-1, true}}); Symtab.insert({CachedHashStringRef(Name), {-1, true}});
} }
// Get pre-LTO symbol binding for -wrap and -defsym symbols.
// If it's in the symbol table, read it from the symbol.
// Otherwise an undef for it will be created with STB_GLOBAL.
static uint8_t getSymbolBinding(SymbolBody *SB) {
pccUnsubmitted
Done
ReplyInline Actions

Is this function necessary? All callers pass a non-null SymbolBody, so they can just query the binding directly.

pcc: Is this function necessary? All callers pass a non-null SymbolBody, so they can just query the…
uint8_t Binding = STB_GLOBAL;
if (SB) {
davideUnsubmitted
Not Done
ReplyInline Actions

Can you explain why you need all three?

davide: Can you explain why you need all three?
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

We need all three if we want to match non-LTO behavior.

  • _wrap_bar is needed for the case where it is defined by the user but may be eliminated during LTO.
  • _real_bar is needed for the test case in the PR: the direct call to _real_bar() from foo() will be bound and inlined by LTO to the user-defined _real_bar() instead of bar() if _real_bar() is not on the list.
  • similarly, if bar() is not on the list, the call to bar() from foo() will be inlined instead of being routed to _wrap_bar().
dmikulin: We need all three if we want to match non-LTO behavior. * _wrap_bar is needed for the case…
davideUnsubmitted
Not Done
ReplyInline Actions

Can you please add a test showing when 3) happens?

davide: Can you please add a test showing when 3) happens?
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions
  1. is also demonstrated by the test case in the PR. I'm working on regression tests for all known cases and will submit them with the new version of the patch.
dmikulin: 3) is also demonstrated by the test case in the PR. I'm working on regression tests for all…
pccUnsubmitted
Not Done
ReplyInline Actions

_wrap_bar is needed for the case where it is defined by the user but may be eliminated during LTO.

This one only requires VisibleToRegularObj, doesn't it?

pcc: > _wrap_bar is needed for the case where it is defined by the user but may be eliminated during…
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

Looks like it. Do we want to handle _wrap_* symbols specially and not set the weak binding for LTO?

dmikulin: Looks like it. Do we want to handle _wrap_* symbols specially and not set the weak binding for…
pccUnsubmitted
Not Done
ReplyInline Actions

Up to you. Note that you can make LTO keep a symbol by calling addUndefined with the symbol name, so it wouldn't be *that* special.

pcc: Up to you. Note that you can make LTO keep a symbol by calling `addUndefined` with the symbol…
Symbol *S = SB->symbol();
Binding = S->Binding;
}
return Binding;
pccUnsubmitted
Not Done
ReplyInline Actions

What about the rename target? If I pass --defsym=foo=bar, the linker should not allow IPO past foo, right?

pcc: What about the rename target? If I pass `--defsym=foo=bar`, the linker should not allow IPO…
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

Right. I'll add it to the list.

dmikulin: Right. I'll add it to the list.
}
// Rename SYM as __wrap_SYM. The original symbol is preserved as __real_SYM. // Rename SYM as __wrap_SYM. The original symbol is preserved as __real_SYM.
// Used to implement --wrap. // Used to implement --wrap.
template <class ELFT> void SymbolTable<ELFT>::wrap(StringRef Name) { template <class ELFT> void SymbolTable<ELFT>::addSymbolWrap(StringRef Name) {
SymbolBody *B = find(Name); SymbolBody *SB = find(Name);
pccUnsubmitted
Done
ReplyInline Actions

I'd minimise the diff here and rename this variable back.

pcc: I'd minimise the diff here and rename this variable back.
if (!B) if (!SB)
return; return;
Symbol *Sym = B->symbol(); Symbol *Sym = SB->symbol();
Symbol *Real = addUndefined(Saver.save("__real_" + Name)); Symbol *Real = addUndefined(Saver.save("__real_" + Name));
Symbol *Wrap = addUndefined(Saver.save("__wrap_" + Name)); Symbol *Wrap = addUndefined(Saver.save("__wrap_" + Name));
// Tell LTO not to eliminate this symbol
pccUnsubmitted
Not Done
ReplyInline Actions

You mean Alias here, right?

pcc: You mean `Alias` here, right?
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

Of course.

dmikulin: Of course.
ruiuUnsubmitted
Done
ReplyInline Actions

Add a blank line before a comment.

ruiu: Add a blank line before a comment.
Wrap->IsUsedInRegularObj = true;
davideUnsubmitted
Done
ReplyInline Actions

You're setting IsUsedInRegularObj from what I can see, here, for all the symbols passed with --wrap.
Can you check this is really NFC? IIRC it could impact the way symbols are added (or not) to the symbol table.

davide: You're setting `IsUsedInRegularObj` from what I can see, here, for all the symbols passed with…
// We rename symbols by replacing the old symbol's SymbolBody with the new Config->RenamedSymbols[Real] =
// symbol's SymbolBody. This causes all SymbolBody pointers referring to the RenamedSymbol{Sym, getSymbolBinding(Real->body())};
// old symbol to instead refer to the new symbol. Config->RenamedSymbols[Sym] =
pccUnsubmitted
Done
ReplyInline Actions

Move this comment to applySymbolRenames.

pcc: Move this comment to applySymbolRenames.
memcpy(Real->Body.buffer, Sym->Body.buffer, sizeof(Sym->Body)); RenamedSymbol{Wrap, getSymbolBinding(Sym->body())};
memcpy(Sym->Body.buffer, Wrap->Body.buffer, sizeof(Wrap->Body));
} }
// Creates alias for symbol. Used to implement --defsym=ALIAS=SYM. // Creates alias for symbol. Used to implement --defsym=ALIAS=SYM.
template <class ELFT> template <class ELFT> void SymbolTable<ELFT>::addSymbolAlias(StringRef Alias,
davideUnsubmitted
Done
ReplyInline Actions

Is this bit needed? I don't see a test for it.

davide: Is this bit needed? I don't see a test for it.
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

It was moved here from SymbolTable::alias, there is a test case for it already.

dmikulin: It was moved here from SymbolTable::alias, there is a test case for it already.
void SymbolTable<ELFT>::alias(StringRef Alias, StringRef Name) { StringRef Name) {
davideUnsubmitted
Not Done
ReplyInline Actions

Can this ever be null? If so, you should check for it.
If not, you may as well inline this in the single use.

davide: Can this ever be null? If so, you should check for it. If not, you may as well inline this in…
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

It can't be null, I just find it easier to read, that's all.

dmikulin: It can't be null, I just find it easier to read, that's all.
SymbolBody *B = find(Name); SymbolBody *SB = find(Name);
if (!B) { if (!SB) {
error("-defsym: undefined symbol: " + Name); error("-defsym: undefined symbol: " + Name);
return; return;
} }
Symbol *Sym = B->symbol(); Symbol *Sym = SB->symbol();
Symbol *AliasSym = addUndefined(Alias); Symbol *AliasSym = addUndefined(Alias);
memcpy(AliasSym->Body.buffer, Sym->Body.buffer, sizeof(AliasSym->Body)); // Tell LTO not to eliminate this symbol
Sym->IsUsedInRegularObj = true;
davideUnsubmitted
Done
ReplyInline Actions

Same here.

davide: Same here.
Config->RenamedSymbols[AliasSym] =
RenamedSymbol{Sym, getSymbolBinding(AliasSym->body())};
}
// Apply symbol renames created by -wrap and -defsym
ruiuUnsubmitted
Done
ReplyInline Actions

It is not obvious why you need this function from the source code. You need to add a comment.

ruiu: It is not obvious why you need this function from the source code. You need to add a comment.
template <class ELFT> void SymbolTable<ELFT>::applySymbolRenames() {
ruiuUnsubmitted
Done
ReplyInline Actions

What is RSI? I found just KV (short for key-value) makes sense in most cases.

ruiu: What is RSI? I found just `KV` (short for key-value) makes sense in most cases.
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

RSI is supposed to be Renamed Symbol Iterator, but KV is ok with me too

dmikulin: RSI is supposed to be Renamed Symbol Iterator, but KV is ok with me too
for (auto &RSI : Config->RenamedSymbols) {
Symbol *Sym = RSI.first;
Symbol *Rename = RSI.second.Target;
Sym->Binding = RSI.second.OrigBinding;
ruiuUnsubmitted
Done
ReplyInline Actions

Ditto

ruiu: Ditto
memcpy(Sym->Body.buffer, Rename->Body.buffer, sizeof(Sym->Body));
}
ruiuUnsubmitted
Done
ReplyInline Actions

Add a blank line before a comment.

ruiu: Add a blank line before a comment.
} }
pccUnsubmitted
Not Done
ReplyInline Actions

Can we now replace these functions with something that applies the rename to each symbol in Config->RenamedSymbols? I think it should be possible with this change to how we declare RenamedSymbols.

struct RenamedSymbol {
  uint8_t OrigBinding;
  Symbol *Target;
};
MapVector<Symbol*, RenamedSymbol> RenamedSymbols;
pcc: Can we now replace these functions with something that applies the rename to each symbol in…
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

But __wrap_ symbols are different: they need to be on the list, but they are not being replaced.

dmikulin: But __wrap_ symbols are different: they need to be on the list, but they are not being replaced.
pccUnsubmitted
Not Done
ReplyInline Actions

They only need to be marked as IsUsedInRegularObj. And that's taken care of for you by addUndefined.

pcc: They only need to be marked as `IsUsedInRegularObj`. And that's taken care of for you by…
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

Map it to NULL?

dmikulin: Map it to NULL?
pccUnsubmitted
Not Done
ReplyInline Actions

I mean that you can just set IsUsedInRegularObj on the __wrap_ symbol before you assign it to Target. Then LTO will keep it alive. There's no need to add it (as a key) to RenamedSymbols then.
http://llvm-cs.pcc.me.uk/tools/lld/ELF/LTO.cpp#136

pcc: I mean that you can just set `IsUsedInRegularObj` on the `__wrap_` symbol before you assign it…
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

This works fine for -wrap, but not for -defsym.

For -wrap, we want to keep the __real_ and the original symbols on the list, so the mappings are:
Real --> Orig and Orig --> Wrap
After we're done with LTO, we copy targets of the mapping back to their keys, Orig --> Real and Wrap --> Orig.

For -defsym, we want to preserve the original symbol, so we would add Orig --> Alias mapping to communicate with LTO. But after we're done, we need to copy Orig --> Alias. I added a flag to the mapping structure to reverse the direction of the copy, so now in the -defsym test the call to bar3() gets inlined and the call to bar2() goes to its alias, bar3. This sounds reasonable to me, just want to make sure it's acceptable.

dmikulin: This works fine for -wrap, but not for -defsym. For -wrap, we want to keep the `__real_` and…
static uint8_t getMinVisibility(uint8_t VA, uint8_t VB) { static uint8_t getMinVisibility(uint8_t VA, uint8_t VB) {
if (VA == STV_DEFAULT) if (VA == STV_DEFAULT)
return VB; return VB;
if (VB == STV_DEFAULT) if (VB == STV_DEFAULT)
return VA; return VA;
return std::min(VA, VB); return std::min(VA, VB);
} }
▲ Show 20 Lines • Show All 126 Lines • ▼ Show 20 Linesstatic int compareDefinedNonCommon(Symbol *S, bool WasInserted, uint8_t Binding,
bool IsAbsolute, typename ELFT::uint Value) { bool IsAbsolute, typename ELFT::uint Value) {
if (int Cmp = compareDefined(S, WasInserted, Binding)) { if (int Cmp = compareDefined(S, WasInserted, Binding)) {
if (Cmp > 0) if (Cmp > 0)
S->Binding = Binding; S->Binding = Binding;
return Cmp; return Cmp;
} }
SymbolBody *B = S->body(); SymbolBody *B = S->body();
if (isa<DefinedCommon>(B)) { if (isa<DefinedCommon>(B)) {
// Non-common symbols take precedence over common symbols. // Non-common symbols take precedence over common symbols.
if (Config->WarnCommon) if (Config->WarnCommon)
warn("common " + S->body()->getName() + " is overridden"); warn("common " + S->body()->getName() + " is overridden");
davideUnsubmitted
Not Done
ReplyInline Actions

empty ifs are not really common in llvm.

davide: empty ifs are not really common in llvm.
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

I'll fix this. Negating the condition seamed counter-intuitive...

dmikulin: I'll fix this. Negating the condition seamed counter-intuitive...
pccUnsubmitted
Not Done
ReplyInline Actions

Is the purpose of this code just to restore the binding of the original symbol? I think it would be simpler to have a post-LTO step that manually restores the binding for each renamed symbol rather than dealing with it here.

pcc: Is the purpose of this code just to restore the binding of the original symbol? I think it…
dmikulinAuthorUnsubmitted
Not Done
ReplyInline Actions

Yes, it was to restore the binding. I changed it to have a separate pass over renamed symbols after LTO.

dmikulin: Yes, it was to restore the binding. I changed it to have a separate pass over renamed symbols…
return 1; return 1;
} else if (auto *R = dyn_cast<DefinedRegular>(B)) { } else if (auto *R = dyn_cast<DefinedRegular>(B)) {
if (R->Section == nullptr && Binding == STB_GLOBAL && IsAbsolute && if (R->Section == nullptr && Binding == STB_GLOBAL && IsAbsolute &&
R->Value == Value) R->Value == Value)
return -1; return -1;
} }
return 0; return 0;
} }
▲ Show 20 Lines • Show All 398 Lines • Show Last 20 Lines