Page MenuHomePhabricator

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

Authored by MaskRay on Dec 18 2018, 9:59 PM.

Details

Reviewers
emaste
brooks
dim
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.

Diff Detail

Event Timeline

MaskRay created this revision.Dec 18 2018, 9:59 PM
MaskRay marked an inline comment as done.Dec 18 2018, 10:00 PM
MaskRay added inline comments.
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...

atanasyan added inline comments.Dec 18 2018, 11:43 PM
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 updated this revision to Diff 178908.Dec 19 2018, 10:04 AM
MaskRay edited the summary of this revision. (Show Details)

Bring back MIPS special case

MaskRay marked an inline comment as done.Dec 19 2018, 10:05 AM

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.

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?

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 added a subscriber: brad.Dec 23 2018, 3:27 PM

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

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

kib added a subscriber: kib.Jan 30 2019, 5:41 AM

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 abandoned this revision.Mar 6 2019, 5:34 AM

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

Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 5:34 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript