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

[Rui Ueyama via llvm-commits]

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171213/02b9f552/attachment.html>