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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/ELF/Driver.cpp:483
   Config->BsymbolicFunctions = Args.hasArg(OPT_Bsymbolic_functions);
+  Config->DefineCommon = Args.hasArg(OPT_define_common);
   Config->Demangle = getArg(Args, OPT_demangle, OPT_no_demangle, true);
----------------
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;


================
Comment at: lld/ELF/Options.td:48
 
+def define_common: F<"define-common">, HelpText<"Assign space to common symbols">;
+
----------------
Please also define "no-define-common" just like other command line options with -no- variants.


================
Comment at: lld/ELF/SyntheticSections.cpp:1160-1164
     if (const OutputSectionBase *OutSec = getOutputSection(Body))
       ESym->st_shndx = OutSec->SectionIndex;
     else if (isa<DefinedRegular<ELFT>>(Body))
       ESym->st_shndx = SHN_ABS;
+    else if (isa<DefinedCommon>(Body)) {
----------------
nit: since you added `{}` to one `else`, please add `{}` to this `if` and `else if`.


https://reviews.llvm.org/D28984