[PATCH] D41438: [ELF] - Do not handle --defsym as regular linkerscript assignment.

[Rafael Avila de Espindola via llvm-commits]

I quite like the fact that defsym symbols have the same semantics as a
mini linker script.

It looks like it is important for linker scripts to allow a symbol to
be redefined (see D27276).

Why is it important for defsym to error?

Thanks,
Rafael

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar updated this revision to Diff 128096.
> grimar added a comment.
>
> - Rebased, updated.
>
>
> https://reviews.llvm.org/D41438
>
> Files:
>   ELF/LinkerScript.cpp
>   ELF/LinkerScript.h
>   ELF/ScriptParser.cpp
>   test/ELF/linkerscript/symbol-multiple-defs.s
>
>
> Index: test/ELF/linkerscript/symbol-multiple-defs.s
> ===================================================================
> --- test/ELF/linkerscript/symbol-multiple-defs.s
> +++ test/ELF/linkerscript/symbol-multiple-defs.s
> @@ -0,0 +1,14 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
> +
> +# RUN: echo "foo = 0x3;" > %t.script
> +# RUN: ld.lld -o %t1 --script %t.script %t
> +# RUN: llvm-objdump -t %t1 | FileCheck %s --check-prefix=SCRIPT
> +# SCRIPT: 0000000000000003 *ABS* 00000000 foo
> +
> +# RUN: not ld.lld --defsym=foo=0x3 -o %t2 %t 2>&1 \
> +# RUN:  | FileCheck %s --check-prefix=ERR
> +# ERR: duplicate symbol: foo
> +
> +.global foo
> +foo:
> Index: ELF/ScriptParser.cpp
> ===================================================================
> --- ELF/ScriptParser.cpp
> +++ ELF/ScriptParser.cpp
> @@ -277,6 +277,7 @@
>    if (!atEOF())
>      setError("EOF expected, but got " + next());
>    SymbolAssignment *Cmd = make<SymbolAssignment>(Name, E, getCurrentLocation());
> +  Cmd->IsDefsym = true;
>    Script->SectionCommands.push_back(Cmd);
>  }
>  
> Index: ELF/LinkerScript.h
> ===================================================================
> --- ELF/LinkerScript.h
> +++ ELF/LinkerScript.h
> @@ -104,6 +104,9 @@
>    bool Provide = false;
>    bool Hidden = false;
>  
> +  // Flag shows if this command is -defsym assignment.
> +  bool IsDefsym = false;
> +
>    // Holds file name and line number for error reporting.
>    std::string Location;
>  };
> Index: ELF/LinkerScript.cpp
> ===================================================================
> --- ELF/LinkerScript.cpp
> +++ ELF/LinkerScript.cpp
> @@ -129,9 +129,6 @@
>    // Define a symbol.
>    Symbol *Sym;
>    uint8_t Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
> -  std::tie(Sym, std::ignore) = Symtab->insert(Cmd->Name, /*Type*/ 0, Visibility,
> -                                              /*CanOmitFromDynSym*/ false,
> -                                              /*File*/ nullptr);
>    ExprValue Value = Cmd->Expression();
>    SectionBase *Sec = Value.isAbsolute() ? nullptr : Value.Sec;
>  
> @@ -148,8 +145,21 @@
>    // write expressions like this: `alignment = 16; . = ALIGN(., alignment)`.
>    uint64_t SymValue = Value.Sec ? 0 : Value.getValue();
>  
> -  replaceSymbol<Defined>(Sym, nullptr, Cmd->Name, STB_GLOBAL, Visibility,
> -                         STT_NOTYPE, SymValue, 0, Sec);
> +  // Regular linker script assignment may be used to update existent symbol
> +  // value, but --defsym assignment should always create new symbol and report
> +  // duplicate symbol definition error if symbol already exists.
> +  if (Cmd->IsDefsym) {
> +    Sym = Symtab->addRegular(Cmd->Name, Visibility, STT_NOTYPE, SymValue,
> +                             /*Size=*/0, STB_GLOBAL, Sec,
> +                             /*File=*/nullptr);
> +  } else {
> +    std::tie(Sym, std::ignore) =
> +        Symtab->insert(Cmd->Name, /*Type*/ 0, Visibility,
> +                       /*CanOmitFromDynSym*/ false,
> +                       /*File*/ nullptr);
> +    replaceSymbol<Defined>(Sym, nullptr, Cmd->Name, STB_GLOBAL, Visibility,
> +                           STT_NOTYPE, SymValue, 0, Sec);
> +  }
>    Cmd->Sym = cast<Defined>(Sym);
>  }
>  
>
>
> Index: test/ELF/linkerscript/symbol-multiple-defs.s
> ===================================================================
> --- test/ELF/linkerscript/symbol-multiple-defs.s
> +++ test/ELF/linkerscript/symbol-multiple-defs.s
> @@ -0,0 +1,14 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
> +
> +# RUN: echo "foo = 0x3;" > %t.script
> +# RUN: ld.lld -o %t1 --script %t.script %t
> +# RUN: llvm-objdump -t %t1 | FileCheck %s --check-prefix=SCRIPT
> +# SCRIPT: 0000000000000003 *ABS* 00000000 foo
> +
> +# RUN: not ld.lld --defsym=foo=0x3 -o %t2 %t 2>&1 \
> +# RUN:  | FileCheck %s --check-prefix=ERR
> +# ERR: duplicate symbol: foo
> +
> +.global foo
> +foo:
> Index: ELF/ScriptParser.cpp
> ===================================================================
> --- ELF/ScriptParser.cpp
> +++ ELF/ScriptParser.cpp
> @@ -277,6 +277,7 @@
>    if (!atEOF())
>      setError("EOF expected, but got " + next());
>    SymbolAssignment *Cmd = make<SymbolAssignment>(Name, E, getCurrentLocation());
> +  Cmd->IsDefsym = true;
>    Script->SectionCommands.push_back(Cmd);
>  }
>  
> Index: ELF/LinkerScript.h
> ===================================================================
> --- ELF/LinkerScript.h
> +++ ELF/LinkerScript.h
> @@ -104,6 +104,9 @@
>    bool Provide = false;
>    bool Hidden = false;
>  
> +  // Flag shows if this command is -defsym assignment.
> +  bool IsDefsym = false;
> +
>    // Holds file name and line number for error reporting.
>    std::string Location;
>  };
> Index: ELF/LinkerScript.cpp
> ===================================================================
> --- ELF/LinkerScript.cpp
> +++ ELF/LinkerScript.cpp
> @@ -129,9 +129,6 @@
>    // Define a symbol.
>    Symbol *Sym;
>    uint8_t Visibility = Cmd->Hidden ? STV_HIDDEN : STV_DEFAULT;
> -  std::tie(Sym, std::ignore) = Symtab->insert(Cmd->Name, /*Type*/ 0, Visibility,
> -                                              /*CanOmitFromDynSym*/ false,
> -                                              /*File*/ nullptr);
>    ExprValue Value = Cmd->Expression();
>    SectionBase *Sec = Value.isAbsolute() ? nullptr : Value.Sec;
>  
> @@ -148,8 +145,21 @@
>    // write expressions like this: `alignment = 16; . = ALIGN(., alignment)`.
>    uint64_t SymValue = Value.Sec ? 0 : Value.getValue();
>  
> -  replaceSymbol<Defined>(Sym, nullptr, Cmd->Name, STB_GLOBAL, Visibility,
> -                         STT_NOTYPE, SymValue, 0, Sec);
> +  // Regular linker script assignment may be used to update existent symbol
> +  // value, but --defsym assignment should always create new symbol and report
> +  // duplicate symbol definition error if symbol already exists.
> +  if (Cmd->IsDefsym) {
> +    Sym = Symtab->addRegular(Cmd->Name, Visibility, STT_NOTYPE, SymValue,
> +                             /*Size=*/0, STB_GLOBAL, Sec,
> +                             /*File=*/nullptr);
> +  } else {
> +    std::tie(Sym, std::ignore) =
> +        Symtab->insert(Cmd->Name, /*Type*/ 0, Visibility,
> +                       /*CanOmitFromDynSym*/ false,
> +                       /*File*/ nullptr);
> +    replaceSymbol<Defined>(Sym, nullptr, Cmd->Name, STB_GLOBAL, Visibility,
> +                           STT_NOTYPE, SymValue, 0, Sec);
> +  }
>    Cmd->Sym = cast<Defined>(Sym);
>  }
>