[lld] r313372 - Keep some relocations with undefined weak symbols.
Peter Smith via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 12 10:44:48 PDT 2017
Apologies it is late in the day, apologies for the drive by comment.
The AArch64 ABI has the words "On platforms that do not support
dynamic pre-emption of symbols an unresolved weak reference to a
symbol relocated by R_AARCH64_CALL26 shall be treated as a jump to the
next instruction (the call becomes a no-op).", there is a similar
passage in the Arm ABI as well.
.weak foo
.global _start
_start:
bl foo
I wouldn't have expected any dynamic content for that particular
example (linked as an executable) unless I'd put some shared object on
the command line or used some flag that implicitly imported undefined
symbols. For example ld.bfd just outputs NOP for that example (an
optimization of skip the function call). From the wording in the ABI
if you are using shared objects on a platform that supports symbol
preemption then it is up to you to get the code for weak references
right, I think if you are on Linux you are supposed to test if the GOT
entry for foo is non-zero and only call the function if it isn't 0.
For background information; the idiom for weak reference that is used
on some bare-metal systems is to do something like:
bl InitOptionalSubSystem1 // Weak reference defined in a static library
bl InitOptionalSubSystem2 // Weak reference defined in a static library
If an OptionalSubSystem is used by a program then the program must
make some non-weak reference to another symbol defined in the
OptionalSubSystem to drag it in from the library, hence defining
InitOptionalSubSystem. If it isn't used the function call is
effectively skipped.
This is very much a static library only use case.
Hope this is of some use
Peter
On 12 October 2017 at 17:56, Rafael Avila de Espindola
<rafael.espindola at gmail.com> wrote:
> Rafael Avila de Espindola <rafael.espindola at gmail.com> writes:
>
>> BTW, I have tried to change this to always create a dynamic symbol table
>> first and remove it in removeUnusedSyntheticSections if empty.
>>
>> A change is that we would have a dynamic symbol table on otherwise
>> static programs if they had an undefined weak symbol. As far as I can
>> tell, that is harmless.
>>
>> An annoying case is what to do with _DYNAMIC. The current patch has two
>> passes over the symbols. First checking if we need anything in the
>> dynamic symbol table. After that pass removeUnusedSyntheticSections can
>> remove the dynamic sections (.dynsym, .dynamic, etc) and we can decide
>> if _DYNAMIC should be added to the staic symbol table.
>>
>> A wip patch is attached. Does anyone think this is a good idea?
>>
>> And yet another option is to always have a .dynsym table. The extra cost
>> on static binaries would be really small, so this is tempting.
>
> I think I found a case that would not work with both options.
>
> In both cases the program
>
> .weak foo
> .global _start
> _start:
> bl foo
>
> Would end up with a dynamic symbol table and a relocation to foo. That
> is exactly what we want if a dynamic linker is to find foo, but I think
> it would break aarch64 programs that expect not having a dynamic linker.
>
> The issue is that on aarch64 (and arm), a call to a undefined weak
> symbol is required to turn into a nop. If we have a dynamic symbol table
> the above is a plt call, and we need the dynamic linker to write a value
> to the .got.
>
> This means we need to known if a dynamic linker will be present or
> not. That is what HasDynSymTab is.
>
> The last version of the patch before I noticed the above issues is
> attached.
>
>
> diff --git a/ELF/Config.h b/ELF/Config.h
> index 83c25faa3..57518bd2e 100644
> --- a/ELF/Config.h
> +++ b/ELF/Config.h
> @@ -120,7 +120,6 @@ struct Configuration {
> bool GdbIndex;
> bool GnuHash = false;
> bool HasDynamicList = false;
> - bool HasDynSymTab;
> bool ICF;
> bool MipsN32Abi = false;
> bool NoGnuUnique;
> diff --git a/ELF/Driver.cpp b/ELF/Driver.cpp
> index 04c1c3fc5..39b8608bf 100644
> --- a/ELF/Driver.cpp
> +++ b/ELF/Driver.cpp
> @@ -1024,15 +1024,6 @@ template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
> for (InputFile *F : Files)
> Symtab->addFile<ELFT>(F);
>
> - // Now that we have every file, we can decide if we will need a
> - // dynamic symbol table.
> - // We need one if we were asked to export dynamic symbols or if we are
> - // producing a shared library.
> - // We also need one if any shared libraries are used and for pie executables
> - // (probably because the dynamic linker needs it).
> - Config->HasDynSymTab =
> - !SharedFiles.empty() || Config->Pic || Config->ExportDynamic;
> -
> // Some symbols (such as __ehdr_start) are defined lazily only when there
> // are undefined symbols for them, so we add these to trigger that logic.
> for (StringRef Sym : Script->ReferencedSymbols)
> diff --git a/ELF/Symbols.cpp b/ELF/Symbols.cpp
> index 934358f6d..00315e06a 100644
> --- a/ELF/Symbols.cpp
> +++ b/ELF/Symbols.cpp
> @@ -338,8 +338,6 @@ uint8_t Symbol::computeBinding() const {
> }
>
> bool Symbol::includeInDynsym() const {
> - if (!Config->HasDynSymTab)
> - return false;
> if (computeBinding() == STB_LOCAL)
> return false;
> if (!body()->isInCurrentDSO())
> diff --git a/ELF/SyntheticSections.cpp b/ELF/SyntheticSections.cpp
> index fc7971e32..07b46cc88 100644
> --- a/ELF/SyntheticSections.cpp
> +++ b/ELF/SyntheticSections.cpp
> @@ -980,6 +980,10 @@ StringTableSection::StringTableSection(StringRef Name, bool Dynamic)
> addString("");
> }
>
> +bool StringTableSection::empty() const {
> + return Dynamic && InX::DynSymTab->empty();
> +}
> +
> // Adds a string to the string table. If HashIt is true we hash and check for
> // duplicates. It is optional because the name of global symbols are already
> // uniqued and hashing them again has a big cost for a small value: uniquing
> @@ -1025,6 +1029,10 @@ DynamicSection<ELFT>::DynamicSection()
> addEntries();
> }
>
> +template <class ELFT> bool DynamicSection<ELFT>::empty() const {
> + return InX::DynSymTab->empty();
> +}
> +
> // There are some dynamic entries that don't depend on other sections.
> // Such entries can be set early.
> template <class ELFT> void DynamicSection<ELFT>::addEntries() {
> @@ -1287,7 +1295,8 @@ template <class ELFT> void RelocationSection<ELFT>::finalizeContents() {
> // If all relocations are *RELATIVE they don't refer to any
> // dynamic symbol and we don't need a dynamic symbol table. If that
> // is the case, just use 0 as the link.
> - this->Link = InX::DynSymTab ? InX::DynSymTab->getParent()->SectionIndex : 0;
> + this->Link =
> + InX::DynSymTab->empty() ? 0 : InX::DynSymTab->getParent()->SectionIndex;
>
> // Set required output section properties.
> getParent()->Link = this->Link;
> @@ -1365,6 +1374,11 @@ void SymbolTableBaseSection::addSymbol(SymbolBody *B) {
> Symbols.push_back({B, StrTabSec.addString(B->getName(), HashIt)});
> }
>
> +bool SymbolTableBaseSection::empty() const {
> + return Type == SHT_DYNSYM && Symbols.empty() && !Config->Pic &&
> + SharedFiles.empty();
> +}
> +
> size_t SymbolTableBaseSection::getSymbolIndex(SymbolBody *Body) {
> // Initializes symbol lookup tables lazily. This is used only
> // for -r or -emit-relocs.
> @@ -1550,6 +1564,8 @@ void GnuHashTableSection::writeBloomFilter(uint8_t *Buf) {
> }
> }
>
> +bool GnuHashTableSection::empty() const { return InX::DynSymTab->empty(); }
> +
> void GnuHashTableSection::writeHashTable(uint8_t *Buf) {
> // Group symbols by hash value.
> std::vector<std::vector<Entry>> Syms(NBuckets);
> @@ -1629,6 +1645,8 @@ void GnuHashTableSection::addSymbols(std::vector<SymbolTableEntry> &V) {
> V.push_back({Ent.Body, Ent.StrTabOffset});
> }
>
> +bool HashTableSection::empty() const { return InX::DynSymTab->empty(); }
> +
> HashTableSection::HashTableSection()
> : SyntheticSection(SHF_ALLOC, SHT_HASH, 4, ".hash") {
> this->Entsize = 4;
> diff --git a/ELF/SyntheticSections.h b/ELF/SyntheticSections.h
> index c6a51ad33..a7734dcf2 100644
> --- a/ELF/SyntheticSections.h
> +++ b/ELF/SyntheticSections.h
> @@ -294,6 +294,7 @@ public:
> void writeTo(uint8_t *Buf) override;
> size_t getSize() const override { return Size; }
> bool isDynamic() const { return Dynamic; }
> + bool empty() const override;
>
> private:
> const bool Dynamic;
> @@ -363,6 +364,7 @@ public:
> DynamicSection();
> void finalizeContents() override;
> void writeTo(uint8_t *Buf) override;
> + bool empty() const override;
> size_t getSize() const override { return Size; }
>
> private:
> @@ -401,6 +403,7 @@ public:
> SymbolTableBaseSection(StringTableSection &StrTabSec);
> void finalizeContents() override;
> void postThunkContents() override;
> + bool empty() const override;
> size_t getSize() const override { return getNumSymbols() * Entsize; }
> void addSymbol(SymbolBody *Body);
> unsigned getNumSymbols() const { return Symbols.size() + 1; }
> @@ -434,6 +437,7 @@ public:
> GnuHashTableSection();
> void finalizeContents() override;
> void writeTo(uint8_t *Buf) override;
> + bool empty() const override;
> size_t getSize() const override { return Size; }
>
> // Adds symbols to the hash table.
> @@ -461,6 +465,7 @@ private:
> class HashTableSection final : public SyntheticSection {
> public:
> HashTableSection();
> + bool empty() const override;
> void finalizeContents() override;
> void writeTo(uint8_t *Buf) override;
> size_t getSize() const override { return Size; }
> diff --git a/ELF/Writer.cpp b/ELF/Writer.cpp
> index e7e880d07..da5352fdb 100644
> --- a/ELF/Writer.cpp
> +++ b/ELF/Writer.cpp
> @@ -291,7 +291,7 @@ template <class ELFT> void Writer<ELFT>::createSyntheticSections() {
>
> // Add MIPS-specific sections.
> if (Config->EMachine == EM_MIPS) {
> - if (!Config->Shared && Config->HasDynSymTab) {
> + if (!Config->Shared) {
> InX::MipsRldMap = make<MipsRldMapSection>();
> Add(InX::MipsRldMap);
> }
> @@ -303,36 +303,34 @@ template <class ELFT> void Writer<ELFT>::createSyntheticSections() {
> Add(Sec);
> }
>
> - if (Config->HasDynSymTab) {
> - InX::DynSymTab = make<SymbolTableSection<ELFT>>(*InX::DynStrTab);
> - Add(InX::DynSymTab);
> + InX::DynSymTab = make<SymbolTableSection<ELFT>>(*InX::DynStrTab);
> + Add(InX::DynSymTab);
>
> - In<ELFT>::VerSym = make<VersionTableSection<ELFT>>();
> - Add(In<ELFT>::VerSym);
> + In<ELFT>::VerSym = make<VersionTableSection<ELFT>>();
> + Add(In<ELFT>::VerSym);
>
> - if (!Config->VersionDefinitions.empty()) {
> - In<ELFT>::VerDef = make<VersionDefinitionSection<ELFT>>();
> - Add(In<ELFT>::VerDef);
> - }
> -
> - In<ELFT>::VerNeed = make<VersionNeedSection<ELFT>>();
> - Add(In<ELFT>::VerNeed);
> + if (!Config->VersionDefinitions.empty()) {
> + In<ELFT>::VerDef = make<VersionDefinitionSection<ELFT>>();
> + Add(In<ELFT>::VerDef);
> + }
>
> - if (Config->GnuHash) {
> - InX::GnuHashTab = make<GnuHashTableSection>();
> - Add(InX::GnuHashTab);
> - }
> + In<ELFT>::VerNeed = make<VersionNeedSection<ELFT>>();
> + Add(In<ELFT>::VerNeed);
>
> - if (Config->SysvHash) {
> - InX::HashTab = make<HashTableSection>();
> - Add(InX::HashTab);
> - }
> + if (Config->GnuHash) {
> + InX::GnuHashTab = make<GnuHashTableSection>();
> + Add(InX::GnuHashTab);
> + }
>
> - Add(InX::Dynamic);
> - Add(InX::DynStrTab);
> - Add(In<ELFT>::RelaDyn);
> + if (Config->SysvHash) {
> + InX::HashTab = make<HashTableSection>();
> + Add(InX::HashTab);
> }
>
> + Add(InX::Dynamic);
> + Add(InX::DynStrTab);
> + Add(In<ELFT>::RelaDyn);
> +
> // Add .got. MIPS' .got is so different from the other archs,
> // it has its own class.
> if (Config->EMachine == EM_MIPS) {
> @@ -1178,6 +1176,7 @@ static void removeUnusedSyntheticSections() {
> OutputSection *OS = SS->getParent();
> if (!SS->empty() || !OS)
> continue;
> + SS->Live = false;
>
> std::vector<BaseCommand *>::iterator Empty = OS->SectionCommands.end();
> for (auto I = OS->SectionCommands.begin(), E = OS->SectionCommands.end();
> @@ -1283,9 +1282,6 @@ template <class ELFT> void Writer<ELFT>::finalizeSections() {
>
> if (!includeInSymtab(*Body))
> continue;
> - if (InX::SymTab)
> - InX::SymTab->addSymbol(Body);
> -
> if (InX::DynSymTab && S->includeInDynsym()) {
> InX::DynSymTab->addSymbol(Body);
> if (auto *SS = dyn_cast<SharedSymbol>(Body))
> @@ -1294,12 +1290,22 @@ template <class ELFT> void Writer<ELFT>::finalizeSections() {
> }
> }
>
> + removeUnusedSyntheticSections();
> +
> + for (Symbol *S : Symtab->getSymbols()) {
> + SymbolBody *Body = S->body();
> +
> + if (!includeInSymtab(*Body))
> + continue;
> + if (InX::SymTab)
> + InX::SymTab->addSymbol(Body);
> + }
> +
> // Do not proceed if there was an undefined symbol.
> if (ErrorCount)
> return;
>
> addPredefinedSections();
> - removeUnusedSyntheticSections();
>
> sortSections();
> Script->removeEmptyCommands();
> @@ -1526,7 +1532,7 @@ template <class ELFT> std::vector<PhdrEntry *> Writer<ELFT>::createPhdrs() {
> Ret.push_back(TlsHdr);
>
> // Add an entry for .dynamic.
> - if (InX::DynSymTab)
> + if (!InX::DynSymTab->empty())
> AddHdr(PT_DYNAMIC, InX::Dynamic->getParent()->getPhdrFlags())
> ->add(InX::Dynamic->getParent());
>
> diff --git a/test/ELF/no-inhibit-exec.s b/test/ELF/no-inhibit-exec.s
> index afb7aed94..3174ef341 100644
> --- a/test/ELF/no-inhibit-exec.s
> +++ b/test/ELF/no-inhibit-exec.s
> @@ -7,9 +7,12 @@
>
> # CHECK: Disassembly of section .text:
> # CHECK-NEXT: _start
> -# CHECK-NEXT: 201000: {{.*}} callq -2101253
> +# CHECK-NEXT: 201000: {{.*}} callq 0
>
> # RELOC: Relocations [
> +# RELOC-NEXT: Section ({{.*}}) .rela.dyn {
> +# RELOC-NEXT: R_X86_64_PC32 _bar 0xFFFFFFFFFFFFFFFC
> +# RELOC-NEXT: }
> # RELOC-NEXT: ]
>
> # next code will not link without noinhibit-exec flag
> diff --git a/test/ELF/symbols.s b/test/ELF/symbols.s
> index 54dfcbd08..d2a38ad04 100644
> --- a/test/ELF/symbols.s
> +++ b/test/ELF/symbols.s
> @@ -50,7 +50,7 @@ internal:
> // CHECK-NEXT: Flags [
> // CHECK-NEXT: SHF_ALLOC
> // CHECK-NEXT: ]
> -// CHECK-NEXT: Address: 0x200158
> +// CHECK-NEXT: Address: 0x20022D
>
> // CHECK: Name: .text
> // CHECK-NEXT: Type: SHT_PROGBITS
> @@ -66,7 +66,7 @@ internal:
> // CHECK-NEXT: SHF_ALLOC
> // CHECK-NEXT: SHF_WRITE
> // CHECK-NEXT: ]
> -// CHECK-NEXT: Address: 0x202000
> +// CHECK-NEXT: Address: 0x203000
> // CHECK-NEXT: Offset:
> // CHECK-NEXT: Size: 4
>
> @@ -82,7 +82,7 @@ internal:
> // CHECK-NEXT: }
> // CHECK-NEXT: Symbol {
> // CHECK-NEXT: Name: hidden
> -// CHECK-NEXT: Value: 0x200160
> +// CHECK-NEXT: Value: 0x200235
> // CHECK-NEXT: Size: 0
> // CHECK-NEXT: Binding: Local
> // CHECK-NEXT: Type: None
> @@ -93,7 +93,7 @@ internal:
> // CHECK-NEXT: }
> // CHECK-NEXT: Symbol {
> // CHECK-NEXT: Name: internal
> -// CHECK-NEXT: Value: 0x200160
> +// CHECK-NEXT: Value: 0x200235
> // CHECK-NEXT: Size: 0
> // CHECK-NEXT: Binding: Local
> // CHECK-NEXT: Type: None
> @@ -103,6 +103,17 @@ internal:
> // CHECK-NEXT: Section: foobar
> // CHECK-NEXT: }
> // CHECK-NEXT: Symbol {
> +// CHECK-NEXT: Name: _DYNAMIC
> +// CHECK-NEXT: Value:
> +// CHECK-NEXT: Size: 0
> +// CHECK-NEXT: Binding: Local
> +// CHECK-NEXT: Type: None
> +// CHECK-NEXT: Other [
> +// CHECK-NEXT: STV_HIDDEN
> +// CHECK-NEXT: ]
> +// CHECK-NEXT: Section: .dynamic
> +// CHECK-NEXT: }
> +// CHECK-NEXT: Symbol {
> // CHECK-NEXT: Name: _start
> // CHECK-NEXT: Value: 0x201000
> // CHECK-NEXT: Size: 0
> @@ -131,7 +142,7 @@ internal:
> // CHECK-NEXT: }
> // CHECK-NEXT: Symbol {
> // CHECK-NEXT: Name: common
> -// CHECK-NEXT: Value: 0x202000
> +// CHECK-NEXT: Value: 0x203000
> // CHECK-NEXT: Size: 4
> // CHECK-NEXT: Binding: Global
> // CHECK-NEXT: Type: Object
> @@ -149,7 +160,7 @@ internal:
> // CHECK-NEXT: }
> // CHECK-NEXT: Symbol {
> // CHECK-NEXT: Name: protected
> -// CHECK-NEXT: Value: 0x200160
> +// CHECK-NEXT: Value: 0x200235
> // CHECK-NEXT: Size: 0
> // CHECK-NEXT: Binding: Global
> // CHECK-NEXT: Type: None
> @@ -160,7 +171,7 @@ internal:
> // CHECK-NEXT: }
> // CHECK-NEXT: Symbol {
> // CHECK-NEXT: Name: zed
> -// CHECK-NEXT: Value: 0x200158
> +// CHECK-NEXT: Value: 0x20022D
> // CHECK-NEXT: Size: 0
> // CHECK-NEXT: Binding: Global (0x1)
> // CHECK-NEXT: Type: None
> @@ -169,7 +180,7 @@ internal:
> // CHECK-NEXT: }
> // CHECK-NEXT: Symbol {
> // CHECK-NEXT: Name: zed2
> -// CHECK-NEXT: Value: 0x20015C
> +// CHECK-NEXT: Value: 0x200231
> // CHECK-NEXT: Size: 0
> // CHECK-NEXT: Binding: Global
> // CHECK-NEXT: Type: None
> @@ -178,7 +189,7 @@ internal:
> // CHECK-NEXT: }
> // CHECK-NEXT: Symbol {
> // CHECK-NEXT: Name: zed3
> -// CHECK-NEXT: Value: 0x200160
> +// CHECK-NEXT: Value: 0x200235
> // CHECK-NEXT: Size: 4
> // CHECK-NEXT: Binding: Global
> // CHECK-NEXT: Type: None
>
>
> Cheers,
> Rafael
>
More information about the llvm-commits
mailing list