[Driver] Use --hash-style=gnu instead of both on FreeBSD

[MaskRay]

Summary

rtld started to support DT_GNU_HASH since rS234841 (2013).
libexec/rtld-elf/rtld.c:symlook_obj uses DT_GNU_HASH when it is present and ignores DT_HASH.
This saves a few hundreds bytes to a few kilobytes for typical executables. (DT_HASH is usually larger than DT_GNU_HASH because it does not skip undefined dynsym entries)

Change the ArchType check with explicit getTripe().isMIPS().
According to atanasyan, ".gnu.hash and the MIPS ABI require .dynsym to be sorted in different ways" so they may not be compatible.

[MaskRay]

lib/Driver/ToolChains/FreeBSD.cpp

160

I can't find rationale behind the MIPS discrepancy in the original commit. I can add the if branch back if you tell me why...

[MaskRay]

.

[atanasyan]

lib/Driver/ToolChains/FreeBSD.cpp

160

I can't find rationale behind the MIPS discrepancy in the original commit. I can add the if branch back if you tell me why...

Did you check that linker on MIPS FreeBSD really support --hash-style=gnu?

As far as I know ".gnu.hash and the MIPS ABI require .dynsym to be sorted in different ways. .gnu.hash needs symbols to be grouped by hash code whereas the MIPS ABI requires a mapping between the GOT and the symbol table".

[MaskRay]

Bring back MIPS special case

[sdardis]

I can't find rationale behind the MIPS discrepancy in the original commit. I can add the if branch back if you tell me why...

From my recollections, the gnu-hash style isn't supported in ld.bfd for MIPS. A patch was posted to the binutils list (https://sourceware.org/ml/binutils/2015-10/msg00057.html) to support this, but there was an issue with FSF copyright assignment. As the patch is non-trivial, it would have to be independently re-developed for submission to binutils and later lld.

[emaste]

I think the arch-change (switching from a whitelist to a MIPS blacklist) is reasonable. What is the motivation for dropping DT_HASH, just binary size reduction?

[MaskRay]

In D55878#1339098, @emaste wrote:

I think the arch-change (switching from a whitelist to a MIPS blacklist) is reasonable. What is the motivation for dropping DT_HASH, just binary size reduction?

Yes. It saves a few hundreds bytes to a few kilobytes for each EXE/DSO.

See Linux.cpp, some Linux distributions default to --hash-style=gnu too:

if (!IsMips && !IsHexagon) {
  if (Distro.IsRedhat() || Distro.IsOpenSUSE() || Distro.IsAlpineLinux() ||
      (Distro.IsUbuntu() && Distro >= Distro::UbuntuMaverick) ||
      (IsAndroid && !Triple.isAndroidVersionLT(23)))
    ExtraOpts.push_back("--hash-style=gnu");

This section is only consumed by dynamic loader. It is unfortunate that OpenBSD only ported this a month ago and illumos does not support it at all. The GNU section is superior to the SYSV counterpart, even in term of section sizes (.hash does not skip local symbols).

[brad]

I've been told there is no desire to make gnu the default on OpenBSD.

[MaskRay]

Friendly ping :) (This is related to FreeBSD, not OpenBSD)

[kib]

I do not like this. The change makes binaries linked by the default toolchain on FreeBSD, non-standard compliant. Several hundred bytes is not too high cost to pay for not having issues with all tools still not adapted to GNU hash (and never be).

[MaskRay]

The DT_GNU_HASH objection is so strong that I think I have to abandon this. I can do nothing but to blame the original sysv hash is so bad (it may be unfair to blame it as it was designed so many years ago) You may be interested in D58102

lib/Driver/ToolChains/FreeBSD.cpp

if (Args.hasArg(options::OPT_static)) {
     if (Args.hasArg(options::OPT_rdynamic))
       CmdArgs.push_back("-export-dynamic");
     if (Args.hasArg(options::OPT_shared)) {
       CmdArgs.push_back("-Bshareable");
     } else {
       CmdArgs.push_back("-dynamic-linker");
       CmdArgs.push_back("/libexec/ld-elf.so.1");
     }
-    if (ToolChain.getTriple().getOSMajorVersion() >= 9) {
-      if (Arch == llvm::Triple::arm || Arch == llvm::Triple::sparc ||
-          Arch == llvm::Triple::x86 || Arch == llvm::Triple::x86_64) {
-        CmdArgs.push_back("--hash-style=both");
-      }
-    }
+    if (ToolChain.getTriple().getOSMajorVersion() >= 9 &&
+        !ToolChain.getTriple().isMIPS())
+      CmdArgs.push_back("--hash-style=gnu");
     CmdArgs.push_back("--enable-new-dtags");
   }
 
   // Explicitly set the linker emulation for platforms that might not
   // be the default emulation for the linker.
   switch (Arch) {
   case llvm::Triple::x86:
     CmdArgs.push_back("-m");

test/Driver/freebsd.c

 // RUN:   --sysroot=%S/Inputs/multiarch_freebsd64_tree -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-LDFLAGS8 %s
 // RUN: %clang -no-canonical-prefixes -target x86_64-pc-freebsd9 -m32 %s \
 // RUN:   --sysroot=%S/Inputs/multiarch_freebsd64_tree -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-LDFLAGS9 %s
 // RUN: %clang -no-canonical-prefixes -target x86_64-pc-freebsd10.0 -m32 %s \
 // RUN:   --sysroot=%S/Inputs/multiarch_freebsd64_tree -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-LDFLAGS9 %s
-// CHECK-LDFLAGS8-NOT: --hash-style=both
+// CHECK-LDFLAGS8-NOT: --hash-style=gnu
 // CHECK-LDFLAGS8: --enable-new-dtags
-// CHECK-LDFLAGS9: --hash-style=both
+// CHECK-LDFLAGS9: --hash-style=gnu
 // CHECK-LDFLAGS9: --enable-new-dtags
 //
 // Check that we do not pass --hash-style=gnu and --hash-style=both to linker
 // and provide correct path to the dynamic linker for MIPS platforms.
 // Also verify that we tell the assembler to target the right ISA and ABI.
 // RUN: %clang %s -### -o %t.o 2>&1 \
 // RUN:     -target mips-unknown-freebsd10.0 \
 // RUN:   | FileCheck --check-prefix=CHECK-MIPS %s