[ELF] Remove obscure -dp and GNU ld incompatible --[no-]define-common, ignore -d/-dc

[MaskRay]

Summary

https://maskray.me/blog/2022-02-06-all-about-common-symbols#no-define-common

In GNU ld, -dc only affects -r links and causes COMMON symbols to be allocated.
--no-define-common is defined to make COMMON symbols undefined for -shared.
AIUI --no-define-common is a workaround around glibc 2.1 time and not really useful.

gold confuses --define-common with -d/FORCE_COMMON_ALLOCATION and implements
--define-common with -d semantics. Its --no-define-common is incompatible with
GNU ld.

In ld.lld, b2a23cf3c08cee45614f27eb2c6d044e506aa6a6 fixed the default -r
behavior for COMMON symbols but ported the incompatible gold
--[no-]define-common. To the best of my knowledge, no project uses -dp
--[no-]define-common. So just remove these options.

-d/-dc are used by the following projects:

• grub grub-core/genmod.sh.in uses -Wl,-r,-d (https://lists.gnu.org/archive/html/grub-devel/2022-02/msg00088.html)
• FreeBSD crunchgen uses -Wl,-dc (https://reviews.freebsd.org/D34215)

A no-op implementation works for them. Only when a program inspects relocatable
output by itself and does not recognize COMMON symbols, there may be a problem.
This is an extremely unlikely case.

[MaskRay]

remove -dc/-dp

I have tried some Debian Code Search patterns and believe they are not used
for targets we support.

• -Wl,-dp -Wl,-dc
• LDFLAGS.*-dc
• LDFLAGS.*-dp

I keep -d because it is used by grub

[peter.smith]

While in this case I think you are right, and no-one using LLD will be depending on this. I'm a bit uncomfortable with taking options out without a bit more warning and chance for feedback. I think it could be worth using this to set out LLD's deprecation/removal process. For example for options that maintainers want to remove we've got several options, or even a combination depending on circumstances:

• Option will be deprecated (with a silenceable warning) for a release, then removed in the subsequent release.
• Option will be removed if there is no use of it in an open source project.
• Option will be removed if a post on the community discourse/mailing lists, github issue has no one feedback with a good reason to keep it within a certain time frame.
• Option will be set to silent ignore if a valid, if sub-optimal output can be produced without applying the option.

I'd strongly recommend that we at least make a github issue (more search engine friendly than a review) and a post on discourse first. Ideally agree some kind of process so that people using LLD can know what to expect.

[MaskRay]

In D119108#3303807, @peter.smith wrote:

While in this case I think you are right, and no-one using LLD will be depending on this. I'm a bit uncomfortable with taking options out without a bit more warning and chance for feedback. I think it could be worth using this to set out LLD's deprecation/removal process. For example for options that maintainers want to remove we've got several options, or even a combination depending on circumstances:

• Option will be deprecated (with a silenceable warning) for a release, then removed in the subsequent release.
• Option will be removed if there is no use of it in an open source project.
• Option will be removed if a post on the community discourse/mailing lists, github issue has no one feedback with a good reason to keep it within a certain time frame.
• Option will be set to silent ignore if a valid, if sub-optimal output can be produced without applying the option.

I'd strongly recommend that we at least make a github issue (more search engine friendly than a review) and a post on discourse first. Ideally agree some kind of process so that people using LLD can know what to expect.

Good idea. Created https://github.com/llvm/llvm-project/issues/53660 & https://discourse.llvm.org/t/remove-gnu-ld-incompatible-no-define-common-and-obscure-dc-dp/59894 as the first step.
Perhaps you may start a post about deprecation policy since you already got the wording:) ?

I think for -dc and -dp we can warn about them in the release/14.x branch. The warning will (a) give us idea whether they are really unused, (b) push projects to remove the options, (c) decide whether we should (actually quite trivial) keep them with full functionality, keep them as ignored options, or remove them altogether.

For release/14.x, I am thinking of this warning:

if (args.hasArg(OPT_define_common, OPT_no_define_common))                                                                                                                                                                      
  warn("-d, -dc, -dp, and --[no-]define-common are ignored and will be removed. See https://github.com/llvm/llvm-project/issues/53660");

[MaskRay]

Keep -dc since used by FreeBSD crunchgen (though being removed by https://reviews.freebsd.org/D34215)

[peter.smith]

LGTM for the changes. It will be worth giving the messages some time to see if there are any others affected.

Perhaps you may start a post about deprecation policy since you already got the wording:) ?

Me and my big mouth! I should have some time on Friday to write a post.

[MaskRay]

Thanks:) 14.0.0 will have this diagnostic: https://github.com/llvm/llvm-project/commit/7efa49801166f221d8fd7b20ac0d04368b8ea9d5

The FreeBSD crunchgen and grub usage of -d/-dc are being removed but we will keep no-op -d/-dc for a while.
-dp and --[no-]define-common will be removed by this patch (milestone: 15.0.0).

[peter.smith]

FYI: Deprecation policy suggestion https://discourse.llvm.org/t/rfc-lld-deprecation-policy/60067

lld/ELF/Config.h

struct Configuration {
   bool asNeeded = false;
   BsymbolicKind bsymbolic = BsymbolicKind::None;
   bool callGraphProfileSort;
   bool checkSections;
   bool checkDynamicRelocs;
   bool compressDebugSections;
   bool cref;
   std::vector<std::pair<llvm::GlobPattern, uint64_t>> deadRelocInNonAlloc;
-  bool defineCommon;
   bool demangle = true;
   bool dependentLibraries;
   bool disableVerify;
   bool ehFrameHdr;
   bool emitLLVM;
   bool emitRelocs;
   bool enableNewDtags;
   bool executeOnly;

lld/ELF/Driver.cpp

if (config->pie && config->shared)
     error("-shared and -pie may not be used together");
 
   if (!config->shared && !config->filterList.empty())
     error("-F may not be used without -shared");
 
   if (!config->shared && !config->auxiliaryList.empty())
     error("-f may not be used without -shared");
 
-  if (!config->relocatable && !config->defineCommon)
-    error("-no-define-common not supported in non relocatable output");
-
   if (config->strip == StripPolicy::All && config->emitRelocs)
     error("--strip-all and --emit-relocs may not be used together");
 
   if (config->zText && config->zIfuncNoplt)
     error("-z text and -z ifunc-noplt may not be used together");
 
   if (config->relocatable) {
     if (config->shared)
if (opt::Arg *arg =
     else if (arg->getOption().matches(OPT_Bsymbolic))
       config->bsymbolic = BsymbolicKind::All;
   }
   config->checkSections =
       args.hasFlag(OPT_check_sections, OPT_no_check_sections, true);
   config->chroot = args.getLastArgValue(OPT_chroot);
   config->compressDebugSections = getCompressDebugSections(args);
   config->cref = args.hasArg(OPT_cref);
-  config->defineCommon = args.hasFlag(OPT_define_common, OPT_no_define_common,
-                                      !args.hasArg(OPT_relocatable));
   config->optimizeBBJumps =
       args.hasFlag(OPT_optimize_bb_jumps, OPT_no_optimize_bb_jumps, false);
   config->demangle = args.hasFlag(OPT_demangle, OPT_no_demangle, true);
   config->dependencyFile = args.getLastArgValue(OPT_dependency_file);
   config->dependentLibraries = args.hasFlag(OPT_dependent_libraries, OPT_no_dependent_libraries, true);
   config->disableVerify = args.hasArg(OPT_disable_verify);
   config->discard = getDiscard(args);
   config->dwoDir = args.getLastArgValue(OPT_plugin_opt_dwo_dir_eq);

lld/ELF/Options.td

defm color_diagnostics: BB<"color-diagnostics",
   "Alias for --color-diagnostics=never">;
 def color_diagnostics_eq: JJ<"color-diagnostics=">,
   HelpText<"Use colors in diagnostics (default: auto)">,
   MetaVarName<"[auto,always,never]">;
 
 def cref: FF<"cref">,
   HelpText<"Output cross reference table. If -Map is specified, print to the map file">;
 
-defm define_common: B<"define-common",
-    "Assign space to common symbols",
-    "Do not assign space to common symbols">;
-
 defm demangle: B<"demangle",
     "Demangle symbol names (default)",
     "Do not demangle symbol names">;
 
 defm dependency_file: EEq<"dependency-file", "Write a dependency file">,
   MetaVarName<"<file>">;
 
 def disable_new_dtags: F<"disable-new-dtags">,

 
 // Aliases
 def: Separate<["-"], "f">, Alias<auxiliary>, HelpText<"Alias for --auxiliary">;
 def: F<"call_shared">, Alias<Bdynamic>, HelpText<"Alias for --Bdynamic">;
 def: F<"dy">, Alias<Bdynamic>, HelpText<"Alias for --Bdynamic">;
 def: F<"dn">, Alias<Bstatic>, HelpText<"Alias for --Bstatic">;
 def: F<"non_shared">, Alias<Bstatic>, HelpText<"Alias for --Bstatic">;
 def: F<"static">, Alias<Bstatic>, HelpText<"Alias for --Bstatic">;
-def: Flag<["-"], "d">, Alias<define_common>, HelpText<"Alias for --define-common">;
-def: F<"dc">, Alias<define_common>, HelpText<"Alias for --define-common">;
-def: F<"dp">, Alias<define_common>, HelpText<"Alias for --define-common">;
 def: Flag<["-"], "x">, Alias<discard_all>, HelpText<"Alias for --discard-all">;
 def: Flag<["-"], "X">, Alias<discard_locals>, HelpText<"Alias for --discard-locals">;
 def: Flag<["-"], "q">, Alias<emit_relocs>, HelpText<"Alias for --emit-relocs">;
 def: Flag<["-"], ")">, Alias<end_group>, HelpText<"Alias for --end-group">;
 def: JoinedOrSeparate<["-"], "e">, Alias<entry>, HelpText<"Alias for --entry">;
 def: Flag<["-"], "E">, Alias<export_dynamic>, HelpText<"Alias for --export-dynamic">;
 def: Separate<["-"], "F">, Alias<filter>, HelpText<"Alias for --filter">;
 def: Separate<["-"], "b">, Alias<format>, HelpText<"Alias for --format">;

 def: J<"plugin-opt=-fresolution=">;
 def: J<"plugin-opt=-pass-through=">;
 // This may be either an unhandled LLVMgold.so feature or GCC passed
 // -plugin-opt=path/to/{liblto_plugin.so,lto-wrapper}
 def plugin_opt_eq : J<"plugin-opt=">;
 
 // Options listed below are silently ignored for now for compatibility.
 def: F<"detect-odr-violations">;
+def: Flag<["-"], "d">;
 def: Flag<["-"], "g">;
 def: F<"long-plt">;
 def: F<"no-copy-dt-needed-entries">;
 def: F<"no-ctors-in-init-array">;
 def: F<"no-keep-memory">;
 def: F<"no-warn-mismatch">;
 def: Separate<["--", "-"], "rpath-link">;
 def: J<"rpath-link=">;

lld/ELF/SyntheticSections.cpp

 
 template <class ELFT>
 SymbolTableSection<ELFT>::SymbolTableSection(StringTableSection &strTabSec)
     : SymbolTableBaseSection(strTabSec) {
   this->entsize = sizeof(Elf_Sym);
 }
 
 static BssSection *getCommonSec(Symbol *sym) {
-  if (!config->defineCommon)
+  if (config->relocatable)
     if (auto *d = dyn_cast<Defined>(sym))
       return dyn_cast_or_null<BssSection>(d->section);
   return nullptr;
 }
 
 static uint32_t getSymSectionIndex(Symbol *sym) {
   assert(!(sym->needsCopy && sym->isObject()));
   if (!isa<Defined>(sym) || sym->needsCopy)
for (SymbolTableEntry &ent : symbols) {
     if (config->emachine == EM_PPC64)
       eSym->st_other |= sym->stOther & 0xe0;
     // The most significant bit of st_other is used by AArch64 ABI for the
     // variant PCS.
     else if (config->emachine == EM_AARCH64)
       eSym->st_other |= sym->stOther & STO_AARCH64_VARIANT_PCS;
 
     if (BssSection *commonSec = getCommonSec(sym)) {
-      // st_value is usually an address of a symbol, but that has a special
-      // meaning for uninstantiated common symbols (--no-define-common).
+      // When -r is specified, a COMMON symbol is not allocated. Its st_shndx
+      // holds SHN_COMMON and st_value holds the alignment.
       eSym->st_shndx = SHN_COMMON;
       eSym->st_value = commonSec->alignment;
       eSym->st_size = cast<Defined>(sym)->size;
     } else {
       const uint32_t shndx = getSymSectionIndex(sym);
       if (isDefinedHere) {
         eSym->st_shndx = shndx;
         eSym->st_value = sym->getVA();

lld/docs/ReleaseNotes.rst

 =================================================
 
 ELF Improvements
 ----------------
 
 Breaking changes
 ----------------
 
-* ...
+* The GNU ld incompatible ``--no-define-common`` has been removed.
+* The obscure ``-d``/``-dc``/``-dp`` options have been removed.
 
 COFF Improvements
 -----------------
 
 * ...
 
 MinGW Improvements
 ------------------

lld/docs/ld.lld.1

 compresses well at that level, but if you want to compress it more,
 you can specify
 .Fl O2
 to set the compression level to 6.
 .It Fl -cref
 Output cross reference table. If
 .Fl Map
 is specified, print to the map file.
-.It Fl -define-common , Fl d
-Assign space to common symbols.
+.It Fl dc
+.Fl r
+specific: assign space to COMMON symbols like building an executable or shared object.
 .It Fl -defsym Ns = Ns Ar symbol Ns = Ns Ar expression
 Define a symbol alias.
 .Ar expression
 may be another symbol or a linker script expression.
 For example,
 .Ql --defsym=foo=bar
 or
 .Ql --defsym=foo=bar+0x100 .

 Do not allow unresolved references in shared libraries.
 This option is enabled by default when linking an executable.
 .It Fl -no-as-needed
 Always set
 .Dv DT_NEEDED
 for shared libraries.
 .It Fl -no-color-diagnostics
 Do not use colors in diagnostics.
-.It Fl -no-define-common
-Do not assign space to common symbols.
 .It Fl -no-demangle
 Do not demangle symbol names.
 .It Fl -no-dynamic-linker
 Inhibit output of an
 .Li .interp
 section.
 .It Fl -no-fortran-common
 Do not search archive members for definitions to override COMMON symbols.

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
 # CHECK-NEXT:     Value: 0x4
 # CHECK-NEXT:     Size: 4
 # CHECK-NEXT:     Binding: Global
 # CHECK-NEXT:     Type: Object
 # CHECK-NEXT:     Other: 0
 # CHECK-NEXT:     Section: Common (0xFFF2)
 # CHECK-NEXT:   }
 
-# DEFCOMM:      Symbol {
-# DEFCOMM:        Name: common
-# DEFCOMM-NEXT:   Value: 0x0
-# DEFCOMM-NEXT:   Size: 4
-# DEFCOMM-NEXT:   Binding: Global
-# DEFCOMM-NEXT:   Type: Object
-# DEFCOMM-NEXT:   Other: 0
-# DEFCOMM-NEXT:   Section: COMMON
-# DEFCOMM-NEXT: }
-
-# RUN: not ld.lld -shared --no-define-common %t1.o -o /dev/null 2>&1 | FileCheck --check-prefix=ERROR %s
-# ERROR: error: -no-define-common not supported in non relocatable output
-
 .comm common,4,4

lld/test/ELF/silent-ignore.test

 RUN: ld.lld --version \
+RUN:   -d \
 RUN:   -detect-odr-violations \
 RUN:   -g \
 RUN:   -long-plt \
 RUN:   -no-copy-dt-needed-entries \
 RUN:   -no-ctors-in-init-array \
 RUN:   -no-keep-memory \
 RUN:   --no-relax \
 RUN:   -no-warn-mismatch \