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

[kettenis]

Summary

Currently ld.lld -r allocates space for common symbols, whereas ld.bfd -r doesn't.
As a result the OpenBSD makefile bits for creating libraries fail as they use ld -X -r to strip local symbols,
which results in duplicate symbol errors because space for the common symbols has been allocated.

The diff also implements the --define-commons option such that allocation of commons can be forced even
if -r is used.

[ruiu]

Can you add a test?

lld/ELF/Config.h

102

Since you always set a value to this variable, please remove = false.

lld/ELF/Symbols.cpp

77

Returning an alignment as an address seems odd. What are you trying to do here?

[kettenis]

h>>! In D28984#652498, @ruiu wrote:

Can you add a test?

Sure.

The alignment is returned as the VA because that's what's being stored in the st_value field of SHN_COMMON symbols.
Alternatively I could return 0 here, and fix up st_value in SymbolTableSection<ELFT>::writeGlobalSymbols(), but this way
seems a little bit cleaner.

[ruiu]

I think I prefer returning 0 from getVA, because that function is supposed to return an address. Returning an alignment seems a bit confusing.

[kettenis]

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.

[ruiu]

lld/ELF/Driver.cpp

483

You added Config->Relocatable && !Config->DefineCommon in multiple places in this patch. I think doing that only once is a good idea.

// -r implies -no-define-common unless -define-common is specified.
if (Args.hasArg(OPT_relocatable) && !Args.hasArg(OPT_define_common))
  Config->DefineCommon = false;

lld/ELF/Options.td

48

Please also define "no-define-common" just like other command line options with -no- variants.

lld/ELF/SyntheticSections.cpp

1160–1168

nit: since you added {} to one else, please add {} to this if and else if.

[kettenis]

New diff that addresses the issues raised by Rui.

[ruiu]

Can you add a test? Otherwise this looks good.

lld/ELF/Driver.cpp

501–502

Nice use of getArg's default value!

[kettenis]

Apologies. Forgot to add a test. The test now also tests the --no-define-common option.

[ruiu]

LGTM. Thanks!

[kettenis]

Thanks. I don't have commit access, so I'd appreciate it if somebody could commit this for me.

[ruiu]

Submitted as r292878.

lld/ELF/Config.h

struct Configuration {
   std::vector<SymbolVersion> VersionScriptGlobals;
   std::vector<SymbolVersion> VersionScriptLocals;
   std::vector<uint8_t> BuildIdVector;
   bool AllowMultipleDefinition;
   bool AsNeeded = false;
   bool Bsymbolic;
   bool BsymbolicFunctions;
   bool ColorDiagnostics = false;
+  bool DefineCommon;
   bool Demangle = true;
   bool DisableVerify;
   bool EhFrameHdr;
   bool EnableNewDtags;
   bool ExportDynamic;
   bool FatalWarnings;
   bool GcSections;
   bool GdbIndex;

lld/ELF/Driver.cpp

std::tie(Config->EKind, Config->EMachine, Config->OSABI) =
         parseEmulation(S);
     Config->MipsN32Abi = (S == "elf32btsmipn32" || S == "elf32ltsmipn32");
     Config->Emulation = S;
   }
 
   Config->AllowMultipleDefinition = Args.hasArg(OPT_allow_multiple_definition);
   Config->Bsymbolic = Args.hasArg(OPT_Bsymbolic);
   Config->BsymbolicFunctions = Args.hasArg(OPT_Bsymbolic_functions);
   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);
   Config->EnableNewDtags = !Args.hasArg(OPT_disable_new_dtags);
   Config->ExportDynamic =
       getArg(Args, OPT_export_dynamic, OPT_no_export_dynamic, false);
   Config->FatalWarnings =
       getArg(Args, OPT_fatal_warnings, OPT_no_fatal_warnings, false);
   Config->GcSections = getArg(Args, OPT_gc_sections, OPT_no_gc_sections, false);
   Config->GdbIndex = Args.hasArg(OPT_gdb_index);
   Config->ICF = Args.hasArg(OPT_icf);
   Config->NoGnuUnique = Args.hasArg(OPT_no_gnu_unique);
   Config->NoUndefinedVersion = Args.hasArg(OPT_no_undefined_version);
   Config->Nostdlib = Args.hasArg(OPT_nostdlib);
   Config->OMagic = Args.hasArg(OPT_omagic);
   Config->Pie = getArg(Args, OPT_pie, OPT_nopie, false);
   Config->PrintGcSections = Args.hasArg(OPT_print_gc_sections);
   Config->Relocatable = Args.hasArg(OPT_relocatable);
+  Config->DefineCommon = getArg(Args, OPT_define_common, OPT_no_define_common,
+      !Config->Relocatable);
   Config->Discard = getDiscardOption(Args);
   Config->SaveTemps = Args.hasArg(OPT_save_temps);
   Config->SingleRoRx = Args.hasArg(OPT_no_rosegment);
   Config->Shared = Args.hasArg(OPT_shared);
   Config->Target1Rel = getArg(Args, OPT_target1_rel, OPT_target1_abs, false);
   Config->Threads = getArg(Args, OPT_threads, OPT_no_threads, true);
   Config->Trace = Args.hasArg(OPT_trace);
   Config->Verbose = Args.hasArg(OPT_verbose);

lld/ELF/Options.td

def as_needed: F<"as-needed">,
   HelpText<"Only set DT_NEEDED for shared libraries if used">;
 
 def color_diagnostics: F<"color-diagnostics">,
   HelpText<"Use colors in diagnostics">;
 
 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">,
   HelpText<"Disable new dynamic tags">;
 
 def discard_all: F<"discard-all">, HelpText<"Delete all local symbols">;
 
 def discard_locals: F<"discard-locals">,
def nostdlib: F<"nostdlib">,
   HelpText<"Only search directories specified on the command line">;
 
 def no_as_needed: F<"no-as-needed">,
   HelpText<"Always DT_NEEDED for shared libraries">;
 
 def no_color_diagnostics: F<"no-color-diagnostics">,
   HelpText<"Do not use colors in diagnostics">;
 
+def no_define_common: F<"no-define-common">,
+  HelpText<"Do not assign space to common symbols">;
+
 def no_demangle: F<"no-demangle">,
   HelpText<"Do not demangle symbol names">;
 
 def no_export_dynamic: F<"no-export-dynamic">;
 def no_fatal_warnings: F<"no-fatal-warnings">;
 
 def no_gc_sections: F<"no-gc-sections">,
   HelpText<"Disable garbage collection of unused sections">;

 // Aliases
 def alias_auxiliary: Separate<["-"], "f">, Alias<auxiliary>;
 def alias_Bdynamic_call_shared: F<"call_shared">, Alias<Bdynamic>;
 def alias_Bdynamic_dy: F<"dy">, Alias<Bdynamic>;
 def alias_Bstatic_dn: F<"dn">, Alias<Bstatic>;
 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>;
 def alias_entry_e: JoinedOrSeparate<["-"], "e">, Alias<entry>;
 def alias_entry_entry: J<"entry=">, Alias<entry>;
 def alias_error_limit: J<"error-limit=">, Alias<error_limit>;
 def alias_export_dynamic_E: Flag<["-"], "E">, Alias<export_dynamic>;
 def alias_export_dynamic_symbol: J<"export-dynamic-symbol=">,

 def plugin: S<"plugin">;
 def plugin_eq: J<"plugin=">;
 def plugin_opt: S<"plugin-opt">;
 def plugin_opt_eq: J<"plugin-opt=">;
 
 // 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">;
 def no_allow_shlib_undefined: F<"no-allow-shlib-undefined">;
 def no_copy_dt_needed_entries: F<"no-copy-dt-needed-entries">,
   Alias<no_add_needed>;
 def no_dynamic_linker: F<"no-dynamic-linker">;
 def no_mmap_output_file: F<"no-mmap-output-file">;
 def no_warn_common: F<"no-warn-common">;
 def no_warn_mismatch: F<"no-warn-mismatch">;
 def rpath_link: S<"rpath-link">;
 def rpath_link_eq: J<"rpath-link=">;
 def sort_common: F<"sort-common">;
 def stats: F<"stats">;
 def warn_execstack: F<"warn-execstack">;
 def warn_shared_textrel: F<"warn-shared-textrel">;
 def EB : F<"EB">;
 def EL : F<"EL">;
 def G: JoinedOrSeparate<["-"], "G">;
 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>;
 
 // LTO-related options.
 def lto_aa_pipeline: J<"lto-aa-pipeline=">,
   HelpText<"AA pipeline to run during LTO. Used in conjunction with -lto-newpm-passes">;
 def lto_newpm_passes: J<"lto-newpm-passes=">,
   HelpText<"Passes to run during LTO">;
 def lto_partitions: J<"lto-partitions=">,
   HelpText<"Number of LTO codegen partitions">;
 def disable_verify: F<"disable-verify">;
 def mllvm: S<"mllvm">;
 def save_temps: F<"save-temps">;
 def thinlto_jobs: J<"thinlto-jobs=">, HelpText<"Number of ThinLTO jobs">;

lld/ELF/Symbols.cpp

if (D.isTls() && !Config->Relocatable) {
       if (!Out<ELFT>::TlsPhdr)
         fatal(toString(D.File) +
               " has a STT_TLS symbol but doesn't have a PT_TLS section");
       return VA - Out<ELFT>::TlsPhdr->p_vaddr;
     }
     return VA;
   }
   case SymbolBody::DefinedCommonKind:
+    if (!Config->DefineCommon)
+      return 0;
     return In<ELFT>::Common->OutSec->Addr + In<ELFT>::Common->OutSecOff +
            cast<DefinedCommon>(Body).Offset;
   case SymbolBody::SharedKind: {
     auto &SS = cast<SharedSymbol<ELFT>>(Body);
     if (!SS.NeedsCopyOrPltAddr)
       return 0;
     if (SS.isFunc())
       return Body.getPltVA<ELFT>();

lld/ELF/SyntheticSections.cpp

 }
 
 // Find all common symbols and allocate space for them.
 template <class ELFT> InputSection<ELFT> *elf::createCommonSection() {
   auto *Ret = make<InputSection<ELFT>>(SHF_ALLOC | SHF_WRITE, SHT_NOBITS, 1,
                                        ArrayRef<uint8_t>(), "COMMON");
   Ret->Live = true;
 
+  if (!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(),
                    [](const DefinedCommon *A, const DefinedCommon *B) {
                      return A->Alignment > B->Alignment;
                    });
 
   // Assign offsets to symbols.
for (const SymbolTableEntry &S : Symbols) {
     uintX_t Size = Body->getSize<ELFT>();
 
     ESym->setBindingAndType(Body->symbol()->computeBinding(), Type);
     ESym->st_size = Size;
     ESym->st_name = StrOff;
     ESym->setVisibility(Body->symbol()->Visibility);
     ESym->st_value = Body->getVA<ELFT>();
 
-    if (const OutputSectionBase *OutSec = getOutputSection(Body))
+    if (const OutputSectionBase *OutSec = getOutputSection(Body)) {
       ESym->st_shndx = OutSec->SectionIndex;
-    else if (isa<DefinedRegular<ELFT>>(Body))
+    } 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
       // pointer equality by STO_MIPS_PLT flag. That is necessary to help
       // dynamic linker distinguish such symbols and MIPS lazy-binding stubs.
       // https://sourceware.org/ml/binutils/2008-07/txt00000.txt
       if (Body->isInPlt() && Body->NeedsCopyOrPltAddr)
         ESym->st_other |= STO_MIPS_PLT;
       if (Config->Relocatable) {
case SymbolBody::DefinedSyntheticKind:
     return cast<DefinedSynthetic>(Sym)->Section;
   case SymbolBody::DefinedRegularKind: {
     auto &D = cast<DefinedRegular<ELFT>>(*Sym);
     if (D.Section)
       return D.Section->OutSec;
     break;
   }
   case SymbolBody::DefinedCommonKind:
+    if (!Config->DefineCommon)
+      return nullptr;
     return In<ELFT>::Common->OutSec;
   case SymbolBody::SharedKind: {
     auto &SS = cast<SharedSymbol<ELFT>>(*Sym);
     if (SS.needsCopy())
       return SS.getBssSectionForCopy();
     break;
   }
   case SymbolBody::UndefinedKind:

lld/test/ELF/relocatable-common.s

+# 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 --no-define-common %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