[PATCH] D28984: [LLD] Do not allocate space for common symbols with -r

[Rafael Avila de Espindola via llvm-commits]

LGTM. Thanks.


Mark Kettenis via Phabricator <reviews at reviews.llvm.org> writes:

> kettenis updated this revision to Diff 85302.
> kettenis added a comment.
>
> New diff that return zero as the VA but sets st_value to the alignment for SHN_COMMON symbols.
> Now also includes a test case.
>
>
> https://reviews.llvm.org/D28984
>
> Files:
>   lld/ELF/Config.h
>   lld/ELF/Driver.cpp
>   lld/ELF/Options.td
>   lld/ELF/Symbols.cpp
>   lld/ELF/SyntheticSections.cpp
>   lld/test/ELF/relocatable-common.s
>
> Index: lld/test/ELF/relocatable-common.s
> ===================================================================
> --- lld/test/ELF/relocatable-common.s
> +++ lld/test/ELF/relocatable-common.s
> @@ -0,0 +1,34 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1.o
> +# RUN: ld.lld -r %t1.o -o %t
> +# RUN: llvm-readobj -symbols -r %t | FileCheck %s
> +# RUN: ld.lld -r --define-common %t1.o -o %t
> +# RUN: llvm-readobj -symbols -r %t | FileCheck -check-prefix=DEFCOMM %s
> +# RUN: ld.lld -r -d %t1.o -o %t
> +# RUN: llvm-readobj -symbols -r %t | FileCheck -check-prefix=DEFCOMM %s
> +# RUN: ld.lld -r -dc %t1.o -o %t
> +# RUN: llvm-readobj -symbols -r %t | FileCheck -check-prefix=DEFCOMM %s
> +# RUN: ld.lld -r -dp %t1.o -o %t
> +# RUN: llvm-readobj -symbols -r %t | FileCheck -check-prefix=DEFCOMM %s
> +
> +# CHECK:        Symbol {
> +# CHECK:          Name: common (1)
> +# CHECK-NEXT:     Value: 0x4
> +# CHECK-NEXT:     Size: 4
> +# CHECK-NEXT:     Binding: Global (0x1)
> +# CHECK-NEXT:     Type: Object (0x1)
> +# CHECK-NEXT:     Other: 0
> +# CHECK-NEXT:     Section: Common (0xFFF2)
> +# CHECK-NEXT:   }
> +
> +# DEFCOMM:      Symbol {
> +# DEFCOMM:        Name: common (1)
> +# DEFCOMM-NEXT:   Value: 0x0
> +# DEFCOMM-NEXT:   Size: 4
> +# DEFCOMM-NEXT:   Binding: Global (0x1)
> +# DEFCOMM-NEXT:   Type: Object (0x1)
> +# DEFCOMM-NEXT:   Other: 0
> +# DEFCOMM-NEXT:   Section: COMMON (0x2)
> +# DEFCOMM-NEXT: }
> +
> +.comm common,4,4
> Index: lld/ELF/SyntheticSections.cpp
> ===================================================================
> --- lld/ELF/SyntheticSections.cpp
> +++ lld/ELF/SyntheticSections.cpp
> @@ -59,6 +59,9 @@
>                                         ArrayRef<uint8_t>(), "COMMON");
>    Ret->Live = true;
>  
> +  if (Config->Relocatable && !Config->DefineCommon)
> +    return Ret;
> +
>    // Sort the common symbols by alignment as an heuristic to pack them better.
>    std::vector<DefinedCommon *> Syms = getCommonSymbols<ELFT>();
>    std::stable_sort(Syms.begin(), Syms.end(),
> @@ -1158,6 +1161,10 @@
>        ESym->st_shndx = OutSec->SectionIndex;
>      else if (isa<DefinedRegular<ELFT>>(Body))
>        ESym->st_shndx = SHN_ABS;
> +    else if (isa<DefinedCommon>(Body)) {
> +      ESym->st_shndx = SHN_COMMON;
> +      ESym->st_value = cast<DefinedCommon>(Body)->Alignment;
> +    }
>  
>      if (Config->EMachine == EM_MIPS) {
>        // On MIPS we need to mark symbol which has a PLT entry and requires
> @@ -1189,6 +1196,8 @@
>      break;
>    }
>    case SymbolBody::DefinedCommonKind:
> +    if (Config->Relocatable && !Config->DefineCommon)
> +      return nullptr;
>      return In<ELFT>::Common->OutSec;
>    case SymbolBody::SharedKind: {
>      auto &SS = cast<SharedSymbol<ELFT>>(*Sym);
> Index: lld/ELF/Symbols.cpp
> ===================================================================
> --- lld/ELF/Symbols.cpp
> +++ lld/ELF/Symbols.cpp
> @@ -73,6 +73,8 @@
>      return VA;
>    }
>    case SymbolBody::DefinedCommonKind:
> +    if (Config->Relocatable && !Config->DefineCommon)
> +      return 0;
>      return In<ELFT>::Common->OutSec->Addr + In<ELFT>::Common->OutSecOff +
>             cast<DefinedCommon>(Body).Offset;
>    case SymbolBody::SharedKind: {
> Index: lld/ELF/Options.td
> ===================================================================
> --- lld/ELF/Options.td
> +++ lld/ELF/Options.td
> @@ -45,6 +45,8 @@
>  def color_diagnostics_eq: J<"color-diagnostics=">,
>    HelpText<"Use colors in diagnostics">;
>  
> +def define_common: F<"define-common">, HelpText<"Assign space to common symbols">;
> +
>  def demangle: F<"demangle">, HelpText<"Demangle symbol names">;
>  
>  def disable_new_dtags: F<"disable-new-dtags">,
> @@ -265,6 +267,9 @@
>  def alias_Bstatic_non_shared: F<"non_shared">, Alias<Bstatic>;
>  def alias_Bstatic_static: F<"static">, Alias<Bstatic>;
>  def alias_L__library_path: J<"library-path=">, Alias<L>;
> +def alias_define_common_d: Flag<["-"], "d">, Alias<define_common>;
> +def alias_define_common_dc: F<"dc">, Alias<define_common>;
> +def alias_define_common_dp: F<"dp">, Alias<define_common>;
>  def alias_discard_all_x: Flag<["-"], "x">, Alias<discard_all>;
>  def alias_discard_locals_X: Flag<["-"], "X">, Alias<discard_locals>;
>  def alias_dynamic_list: J<"dynamic-list=">, Alias<dynamic_list>;
> @@ -332,7 +337,6 @@
>  // Options listed below are silently ignored for now for compatibility.
>  def allow_shlib_undefined: F<"allow-shlib-undefined">;
>  def cref: Flag<["--"], "cref">;
> -def define_common: F<"define-common">;
>  def detect_odr_violations: F<"detect-odr-violations">;
>  def g: Flag<["-"], "g">;
>  def no_add_needed: F<"no-add-needed">;
> @@ -355,9 +359,6 @@
>  def Qy : F<"Qy">;
>  
>  // Aliases for ignored options
> -def alias_define_common_d: Flag<["-"], "d">, Alias<define_common>;
> -def alias_define_common_dc: F<"dc">, Alias<define_common>;
> -def alias_define_common_dp: F<"dp">, Alias<define_common>;
>  def alias_version_script_version_script: J<"version-script=">,
>    Alias<version_script>;
>  
> Index: lld/ELF/Driver.cpp
> ===================================================================
> --- lld/ELF/Driver.cpp
> +++ lld/ELF/Driver.cpp
> @@ -480,6 +480,7 @@
>    Config->AllowMultipleDefinition = Args.hasArg(OPT_allow_multiple_definition);
>    Config->Bsymbolic = Args.hasArg(OPT_Bsymbolic);
>    Config->BsymbolicFunctions = Args.hasArg(OPT_Bsymbolic_functions);
> +  Config->DefineCommon = Args.hasArg(OPT_define_common);
>    Config->Demangle = getArg(Args, OPT_demangle, OPT_no_demangle, true);
>    Config->DisableVerify = Args.hasArg(OPT_disable_verify);
>    Config->EhFrameHdr = Args.hasArg(OPT_eh_frame_hdr);
> Index: lld/ELF/Config.h
> ===================================================================
> --- lld/ELF/Config.h
> +++ lld/ELF/Config.h
> @@ -99,6 +99,7 @@
>    bool Bsymbolic;
>    bool BsymbolicFunctions;
>    bool ColorDiagnostics = false;
> +  bool DefineCommon;
>    bool Demangle = true;
>    bool DisableVerify;
>    bool EhFrameHdr;