[ELF] - Fix for: error "invalid section index: xxx" when linking FreeBSD kernel.

[grimar]

Summary

This is PR28868. More details on a bug page.
Patch fixes the issue by ignoring such "zero" sections.

[davide]

testcase?

[grimar]

In D23201#507140, @davide wrote:

testcase?

hpt27xx_lib.o file used in bug is prebuilt and has about 1.1mb size. I do not know way to reduce it or to produce
smaller sample. Should I use it or we can just rely on the fact that the change works for linking it in BSD world ?

[davide]

In D23201#508362, @grimar wrote:

In D23201#507140, @davide wrote:

testcase?

hpt27xx_lib.o file used in bug is prebuilt and has about 1.1mb size. I do not know way to reduce it or to produce
smaller sample. Should I use it or we can just rely on the fact that the change works for linking it in BSD world ?

No. Not quite. We don't have a FreeBSD buildbot that does CI and even if we had it this code can break at any time and it will require somebody (likely me or Ed Maste) to ask for a revert. Please take the time to reduce this test case.

[ruiu]

As Davide pointed out, it needs a testcase. This is a subtle change, and I'm not sure if this is a bug of LLD.

[grimar]

In D23201#509124, @ruiu wrote:

As Davide pointed out, it needs a testcase. This is a subtle change, and I'm not sure if this is a bug of LLD.

I think it is definely not an issue of lld. But since that file was precompiled in the wild and ld works fine here, I think we also should
support this case, right ?
I`ll try to reduce it and prepare testcase, though I have no idea now how to do that since as I mentioned it is precompiled.

[davide]

In D23201#509541, @grimar wrote:

In D23201#509124, @ruiu wrote:

As Davide pointed out, it needs a testcase. This is a subtle change, and I'm not sure if this is a bug of LLD.

I think it is definely not an issue of lld. But since that file was precompiled in the wild and ld works fine here, I think we also should
support this case, right ?
I`ll try to reduce it and prepare testcase, though I have no idea now how to do that since as I mentioned it is precompiled.

You can probably localize the issue is and create a llvm-mc testcase which reproduces it.
If you can't you can check in a small binary (modified with hexedit or something similar) or use yaml2obj to craft an object of this kind.

[grimar]

• Created testcase.

[ruiu]

Now that I understand what kind of file you are trying to handle. This seems pretty weird. How does this happen? Do you know who created that file?

ELF/InputFiles.cpp

345 ↗

(On Diff #67308)

This is not a good place to fix it. Set InputSectionBase<ELFT>::Discarded to REL/RELA sections instead.

[emaste]

In D23201#510172, @ruiu wrote:

Now that I understand what kind of file you are trying to handle. This seems pretty weird. How does this happen? Do you know who created that file?

I am not certain about hpt27xx_lib.o, but a similar case in FreeBSD was due to the outdated GNU binutils 2.17.50 that we have, and has now been fixed by https://svnweb.freebsd.org/base?view=revision&revision=303158

[ruiu]

Ed, do you happen to know the ld version number that produced wrong object files? I think we are fine with this change, but I'd like to add that information as the comment.

[davide]

In D23201#510474, @ruiu wrote:

Ed, do you happen to know the ld version number that produced wrong object files? I think we are fine with this change, but I'd like to add that information as the comment.

davide@freefall:~ % ld --version
GNU ld 2.17.50 [FreeBSD] 2007-07-03
Copyright 2007 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License.  This program has absolutely no warranty.

[davide]

In D23201#510474, @ruiu wrote:

Ed, do you happen to know the ld version number that produced wrong object files? I think we are fine with this change, but I'd like to add that information as the comment.

Actually, I think you might want the version of as(1). It's the same.

% as --version
GNU assembler 2.17.50 [FreeBSD] 2007-07-03
Copyright 2007 Free Software Foundation, Inc.
This program is free software; you may redistribute it under the terms of
the GNU General Public License.  This program has absolutely no warranty.
This assembler was configured for a target of `x86_64-unknown-freebsd'.

[ruiu]

Thanks, Davide. George, please include that information to this patch.

[grimar]

• Addressed review comments.

[grimar]

ELF/InputFiles.cpp

345 ↗

(On Diff #67308)

Ok, BTW, it also touches .shstrtab, .symtab, .strtab sections in originall issue.
I updated testcase and code.

[ruiu]

ELF/InputFiles.cpp

279–280 ↗

(On Diff #67487)

Hmm, why did you add more conditions?

ELF/Writer.cpp

100–101 ↗

(On Diff #67487)

Why did you have to make a change to this code?

[grimar]

ELF/InputFiles.cpp

279–280 ↗

(On Diff #67487)

Because hpt27xx_lib.o which initially was the reason to fix all of that
contains section symbols associated with
.rela.text, .rela.rodata, .rela.data, .rela.eh_frame, .shstrtab, .symtab, .strtab
See https://llvm.org/bugs/show_bug.cgi?id=28868

Testcase also has them:

• Type: STT_SECTION Section: .rela.text
• Type: STT_SECTION Section: .shstrtab
• Type: STT_SECTION Section: .symtab
• Type: STT_SECTION Section: .strtab

ELF/Writer.cpp

100–101 ↗

(On Diff #67487)

Because since these sections are mo more nullptr, but discarded, the code below is executed
and it is not possible to call IS->getFile()->getName() when IS is InputSection<ELFT>::Discarded,
because File member is null.

[grimar]

By the way, I have to notice that initial version of the patch has only 2 lines of code changed.
May be we can go with it ?

[ruiu]

OK, please revert to the original patch. But please keep the comment. Comments to describe why you needed these changes are as important as the code that does them.

[grimar]

• Reverted code to initial diff.

[ruiu]

LGTM

lld/trunk/ELF/InputFiles.cpp

 }
 
 template <class ELFT>
 InputSectionBase<ELFT> *
 elf::ObjectFile<ELFT>::getSection(const Elf_Sym &Sym) const {
   uint32_t Index = this->getSectionIndex(Sym);
   if (Index == 0)
     return nullptr;
-  if (Index >= Sections.size() || !Sections[Index])
+  if (Index >= Sections.size())
     fatal(getFilename(this) + ": invalid section index: " + Twine(Index));
   InputSectionBase<ELFT> *S = Sections[Index];
-  if (S == &InputSectionBase<ELFT>::Discarded)
+  // We found that GNU assembler 2.17.50 [FreeBSD] 2007-07-03
+  // could generate broken objects. STT_SECTION symbols can be
+  // associated with SHT_REL[A]/SHT_SYMTAB/SHT_STRTAB sections.
+  // In this case it is fine for section to be null here as we
+  // do not allocate sections of these types.
+  if (!S || S == &InputSectionBase<ELFT>::Discarded)
     return S;
   return S->Repl;
 }
 
 template <class ELFT>
 SymbolBody *elf::ObjectFile<ELFT>::createSymbolBody(const Elf_Sym *Sym) {
   int Binding = Sym->getBinding();
   InputSectionBase<ELFT> *Sec = getSection(*Sym);

lld/trunk/test/ELF/section-symbols.test

+# RUN: yaml2obj %s -o %t
+# RUN: ld.lld -shared %t -o %tout
+
+# Verify that lld can handle STT_SECTION symbols associated
+# with SHT_REL[A]/SHT_SYMTAB/SHT_STRTAB sections.
+
+!ELF
+FileHeader:      
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  OSABI:           ELFOSABI_FREEBSD
+  Type:            ET_REL
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .text
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
+    AddressAlign:    0x0000000000000010
+    Content:         "00000000"
+  - Name:            .rela.text
+    Type:            SHT_RELA
+    Link:            .symtab
+    AddressAlign:    0x0000000000000008
+    Info:            .text
+    Relocations:
+Symbols:
+  Local:
+    - Type:            STT_SECTION
+      Section:         .rela.text
+    - Type:            STT_SECTION
+      Section:         .shstrtab
+    - Type:            STT_SECTION
+      Section:         .symtab
+    - Type:            STT_SECTION
+      Section:         .strtab