[lld] r319127 - Store the real binding of shared symbols.

[Rafael Avila de Espindola via llvm-commits]

Looking, sorry about that.


Rui Ueyama <ruiu at google.com> writes:

> This change triggers assertion on one of Chromium bots. Looks like
> `assert(!B.isLocal())` in computeIsPreemptible is failing.
>
> https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.chrome%2FGoogle_Chrome_Linux_x64%2F25172%2F%2B%2Frecipes%2Fsteps%2Fcompile%2F0%2Fstdout
>
> Looking at your change, it looks like we were (and still?) accidentally
> adding shared local symbols to the symbol table as STB_WEAK. My hypothesis
> is that now that they are added with the original binding, the assertion
> starts triggering.
>
> On Mon, Nov 27, 2017 at 5:04 PM, Rafael Espindola via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: rafael
>> Date: Mon Nov 27 17:04:51 2017
>> New Revision: 319127
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=319127&view=rev
>> Log:
>> Store the real binding of shared symbols.
>>
>> Currently we mark every shared symbol as STB_WEAK.
>>
>> That is a hack to make it easy to decide when a .so is needed or not
>> because of a reference to a given symbol.
>>
>> That hack leaks when we create copy relocations as shown by the update
>> to relocation-copy-alias.s.
>>
>> This patch stores the original binding when we first read a shared
>> symbol. We still have to update the binding to weak if we see a weak
>> undef, but I find the logic easier to read where it is now.
>>
>> Modified:
>>     lld/trunk/ELF/SymbolTable.cpp
>>     lld/trunk/ELF/Symbols.h
>>     lld/trunk/test/ELF/relocation-copy-alias.s
>>
>> Modified: lld/trunk/ELF/SymbolTable.cpp
>> URL: http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/
>> SymbolTable.cpp?rev=319127&r1=319126&r2=319127&view=diff
>> ============================================================
>> ==================
>> --- lld/trunk/ELF/SymbolTable.cpp (original)
>> +++ lld/trunk/ELF/SymbolTable.cpp Mon Nov 27 17:04:51 2017
>> @@ -300,9 +300,9 @@ Symbol *SymbolTable::addUndefined(String
>>      replaceSymbol<Undefined>(S, File, Name, Binding, StOther, Type);
>>      return S;
>>    }
>> +  if (S->isShared() || S->isLazy() || (S->isUndefined() && Binding !=
>> STB_WEAK))
>> +    S->Binding = Binding;
>>    if (Binding != STB_WEAK) {
>> -    if (!S->isDefined())
>> -      S->Binding = Binding;
>>      if (auto *SS = dyn_cast<SharedSymbol>(S))
>>        if (!Config->GcSections)
>>          SS->getFile<ELFT>()->IsNeeded = true;
>> @@ -310,12 +310,10 @@ Symbol *SymbolTable::addUndefined(String
>>    if (auto *L = dyn_cast<Lazy>(S)) {
>>      // An undefined weak will not fetch archive members. See comment on
>> Lazy in
>>      // Symbols.h for the details.
>> -    if (Binding == STB_WEAK) {
>> +    if (Binding == STB_WEAK)
>>        L->Type = Type;
>> -      L->Binding = STB_WEAK;
>> -    } else if (InputFile *F = L->fetch()) {
>> +    else if (InputFile *F = L->fetch())
>>        addFile<ELFT>(F);
>> -    }
>>    }
>>    return S;
>>  }
>> @@ -497,8 +495,9 @@ void SymbolTable::addShared(StringRef Na
>>    if (WasInserted || ((S->isUndefined() || S->isLazy()) &&
>>                        S->getVisibility() == STV_DEFAULT)) {
>>      uint8_t Binding = S->Binding;
>> -    replaceSymbol<SharedSymbol>(S, File, Name, Sym.st_other,
>> Sym.getType(),
>> -                                Sym.st_value, Sym.st_size, Alignment,
>> Verdef);
>> +    replaceSymbol<SharedSymbol>(S, File, Name, Sym.getBinding(),
>> Sym.st_other,
>> +                                Sym.getType(), Sym.st_value, Sym.st_size,
>> +                                Alignment, Verdef);
>>      if (!WasInserted) {
>>        S->Binding = Binding;
>>        if (!S->isWeak() && !Config->GcSections)
>>
>> Modified: lld/trunk/ELF/Symbols.h
>> URL: http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Symbols.
>> h?rev=319127&r1=319126&r2=319127&view=diff
>> ============================================================
>> ==================
>> --- lld/trunk/ELF/Symbols.h (original)
>> +++ lld/trunk/ELF/Symbols.h Mon Nov 27 17:04:51 2017
>> @@ -209,10 +209,11 @@ class SharedSymbol : public Symbol {
>>  public:
>>    static bool classof(const Symbol *S) { return S->kind() == SharedKind; }
>>
>> -  SharedSymbol(StringRef Name, uint8_t StOther, uint8_t Type, uint64_t
>> Value,
>> -               uint64_t Size, uint32_t Alignment, const void *Verdef)
>> -      : Symbol(SharedKind, Name, llvm::ELF::STB_WEAK, StOther, Type),
>> -        Verdef(Verdef), Value(Value), Size(Size), Alignment(Alignment) {
>> +  SharedSymbol(StringRef Name, uint8_t Binding, uint8_t StOther, uint8_t
>> Type,
>> +               uint64_t Value, uint64_t Size, uint32_t Alignment,
>> +               const void *Verdef)
>> +      : Symbol(SharedKind, Name, Binding, StOther, Type), Verdef(Verdef),
>> +        Value(Value), Size(Size), Alignment(Alignment) {
>>      // GNU ifunc is a mechanism to allow user-supplied functions to
>>      // resolve PLT slot values at load-time. This is contrary to the
>>      // regualr symbol resolution scheme in which symbols are resolved just
>>
>> Modified: lld/trunk/test/ELF/relocation-copy-alias.s
>> URL: http://llvm.org/viewvc/llvm-project/lld/trunk/test/ELF/
>> relocation-copy-alias.s?rev=319127&r1=319126&r2=319127&view=diff
>> ============================================================
>> ==================
>> --- lld/trunk/test/ELF/relocation-copy-alias.s (original)
>> +++ lld/trunk/test/ELF/relocation-copy-alias.s Mon Nov 27 17:04:51 2017
>> @@ -61,7 +61,7 @@ movl $5, b2
>>  // CHECK:      Name: b3
>>  // CHECK-NEXT: Value: [[B]]
>>  // CHECK-NEXT: Size: 1
>> -// CHECK-NEXT: Binding: Weak
>> +// CHECK-NEXT: Binding: Global
>>  // CHECK-NEXT: Type: Object (0x1)
>>  // CHECK-NEXT: Other: 0
>>  // CHECK-NEXT: Section: .bss
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>