[GNU] Implement --enable-new-dtags/--disable-new-dtags

[davide]

Summary

This fixes https://llvm.org/bugs/show_bug.cgi?id=23036

[davide]

Sample output:

davide@rabbit1:/exps/llvm/build/bin % ./clang test.c -shared -Wl,-rpath=/plop -Wl,--disable-new-dtags -o test1.so
davide@rabbit1:/exps/llvm/build/bin % readelf --dynamic ./test1.so | grep PATH
0x000000000000000f (RPATH) Library rpath: [/plop]
davide@rabbit1:/exps/llvm/build/bin % ./clang test.c -shared -Wl,-rpath=/plop -Wl,--enable-new-dtags -o test1.so
davide@rabbit1:/exps/llvm/build/bin % readelf --dynamic ./test1.so | grep PATH
0x000000000000001d (RUNPATH) Library runpath: [/plop]

[ruiu]

include/lld/ReaderWriter/ELFLinkingContext.h

295

This is not a popular option, so please add more comments on this feature for readers of this code. We should mention that, if the flag is set, we emit DT_RUNPATH instead of DT_RPATH. They are functionally equivalent, but the former has lower precedence than the latter (in terms of other variables like LD_LIBRARY_PATH).

297

Both functions sound like setters of the variable. Maybe we should name the getter getEnableNewDTags? It's not consistent with the others, but in most places we use "get" for getters.

lib/Driver/GnuLdDriver.cpp

629

I prefer this

if (parseArgs->hasArg(OPT_enable_newdtags))
  ctx->setEnableNewDtags(true);

[emaste]

include/lld/ReaderWriter/ELFLinkingContext.h

295

There is another difference according to a thread[1] on switching gold to enable it by default : DT_RUNPATH is used only for direct dependencies, while DT_RPATH is also used for indirect dependencies.

[1] https://sourceware.org/ml/binutils/2014-02/msg00031.html

[bapt]

It is actually a very popular option, it is mostly hidden because for example:

• clang for instance is always passing --enable-new-dtags by default.
• Gentoo activates it by default https://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo/src/patchsets/binutils/2.25/76_all_use-new-ld-dtags.patch?diff_format=s&view=log&pathrev=HEAD

--new-enable-dtags has been made to workaround an issue

• Dragonfly BSD is another example: http://gitweb.dragonflybsd.org/dragonfly.git/commit/0d2ea3af92127a10961ca1d0332ec09bb20d5703

this option has been made to allow to workaround issues with DT_RPATH by allowing to make it respect LD_LIBRARY_PATH useful for example for inplace regression test in build system

[joerg]

It is only popular on system that implemented the same brain-dead LD_LIBRARY_PATH behavior as glibc did.

[ruiu]

Anyways, my point is that the change to the linker for this feature might
be trivial, so it'd be easy to understand what is done here. But it would
not help readers of the code understand why we want to do that. So I wanted
to add a comment on that (a brief comment should suffice).

[davide]

Updated.

[ruiu]

LGTM

lib/ReaderWriter/ELF/OutputELFWriter.h

283

You can remove parentheses around _ctx.getEnableNewDtags().

[emaste]

In D8836#152189, @joerg wrote:

It is only popular on system that implemented the same brain-dead LD_LIBRARY_PATH behavior as glibc did.

Note that the brain-dead LD_LIBRARY_PATH is required by the spec (gabi41.pdf, pg 93). I'm not sure if Solaris actually implemented that though, or if it was just a documentation bug.

I wonder if we should just enable DT_RUNPATH for all targets where we know the rtld supports it.

include/lld/ReaderWriter/ELFLinkingContext.h

public:
   std::string demangle(StringRef symbolName) const override;
   bool demangleSymbols() const { return _demangle; }
   void setDemangleSymbols(bool d) { _demangle = d; }
 
   /// \brief Align segments.
   bool alignSegments() const { return _alignSegments; }
   void setAlignSegments(bool align) { _alignSegments = align; }
 
+  /// \brief Enable new dtags.
+  /// If this flag is set lld emits DT_RUNPATH instead of
+  /// DT_RUNPATH. They are functionally equivalent except for
+  /// the following two differences:
+  /// - DT_RUNPATH is searched after LD_LIBRARY_PATH, while
+  /// DT_RPATH is searched before.
+  /// - DT_RUNPATH is used only to search for direct dependencies
+  /// of the object it's contained in, while DT_RPATH is used
+  /// for indirect dependencies as well.
+  bool getEnableNewDtags() const { return _enableNewDtags; }
+  void setEnableNewDtags(bool e) { _enableNewDtags = e; }
+
   /// \brief Strip symbols.
   bool stripSymbols() const { return _stripSymbols; }
   void setStripSymbols(bool strip) { _stripSymbols = strip; }
 
   /// \brief Collect statistics.
   bool collectStats() const { return _collectStats; }
   void setCollectStats(bool s) { _collectStats = s; }
 
protected:
   bool _mergeCommonStrings = false;
   bool _useShlibUndefines = true;
   bool _dynamicLinkerArg = false;
   bool _noAllowDynamicLibraries = false;
   bool _mergeRODataToTextSegment = true;
   bool _demangle = true;
   bool _stripSymbols = false;
   bool _alignSegments = true;
+  bool _enableNewDtags = false;
   bool _collectStats = false;
   bool _armTarget1Rel = false;
   uint64_t _maxPageSize = 0x1000;
 
   OutputMagic _outputMagic;
   StringRefVector _inputSearchPaths;
   std::unique_ptr<Writer> _writer;
   llvm::Optional<StringRef> _dynamicLinkerPath;

lib/Driver/GnuLdDriver.cpp

bool GnuLdDriver::parse(int argc, const char *argv[],
 
   for (auto *arg : parsedArgs->filtered(OPT_rpath_link)) {
     SmallVector<StringRef, 2> rpaths;
     StringRef(arg->getValue()).split(rpaths, ":");
     for (auto path : rpaths)
       ctx->addRpathLink(path);
   }
 
+  // Enable new dynamic tags.
+  if (parsedArgs->hasArg(OPT_enable_newdtags))
+    ctx->setEnableNewDtags(true);
+
   // Support --wrap option.
   for (auto *arg : parsedArgs->filtered(OPT_wrap))
     ctx->addWrapForSymbol(arg->getValue());
 
   // Register possible input file parsers.
   ctx->registry().addSupportELFObjects(*ctx);
   ctx->registry().addSupportArchives(ctx->logInputFiles());
   ctx->registry().addSupportYamlFiles();
   ctx->registry().addSupportNativeObjects();
   if (ctx->allowLinkWithDynamicLibraries())
     ctx->registry().addSupportELFDynamicSharedObjects(*ctx);

lib/Driver/GnuLdOptions.td

defm defsym : mDashEq<"defsym",
            MetaVarName<"symbol=<expression>">,
            Group<grp_resolveropt>;
 
 //===----------------------------------------------------------------------===//
 /// Custom Options
 //===----------------------------------------------------------------------===//
 def grp_customopts : OptionGroup<"opts">,
      HelpText<"CUSTOM OPTIONS">;
+def disable_newdtags: Flag<["--"], "disable-new-dtags">,
+     HelpText<"Disable new dynamic tags">,
+     Group<grp_customopts>;
+def enable_newdtags: Flag<["--"], "enable-new-dtags">,
+     HelpText<"Enable new dynamic tags">,
+     Group<grp_customopts>;
 def rosegment: Flag<["--"], "rosegment">,
      HelpText<"Put read-only non-executable sections in their own segment">,
      Group<grp_customopts>;
 def z : Separate<["-"], "z">,
      HelpText<"Linker Option extensions">,
      Group<grp_customopts>;
 def no_align_segments: Flag<["--"], "no-align-segments">,
      HelpText<"Don't align ELF segments(virtualaddress/fileoffset) to page boundaries">,

lib/ReaderWriter/ELF/OutputELFWriter.h

for (const auto &loadName : _soNeeded) {
     dyn.d_un.d_val = _dynamicStringTable->addString(loadName.getKey());
     _dynamicTable->addEntry(dyn);
   }
   const auto &rpathList = _ctx.getRpathList();
   if (!rpathList.empty()) {
     auto rpath = new (_alloc) std::string(join(rpathList.begin(),
       rpathList.end(), ":"));
     Elf_Dyn dyn;
-    dyn.d_tag = DT_RPATH;
+    dyn.d_tag = (_ctx.getEnableNewDtags()) ? DT_RUNPATH : DT_RPATH;
     dyn.d_un.d_val = _dynamicStringTable->addString(*rpath);
     _dynamicTable->addEntry(dyn);
   }
   StringRef soname = _ctx.sharedObjectName();
   if (!soname.empty() && _ctx.getOutputELFType() == llvm::ELF::ET_DYN) {
     Elf_Dyn dyn;
     dyn.d_tag = DT_SONAME;
     dyn.d_un.d_val = _dynamicStringTable->addString(soname);