[PATCH] D27131: [ELF] - Add support of proccessing of the rest allocatable synthetic sections from linkerscript.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/LinkerScript.cpp:484-485
     auto *IB = static_cast<InputSectionBase<ELFT> *>(ID);
+    // OutSec can be null when synthetic section listed under
+    // output section command was removed because was not needed.
+    if (!IB->OutSec)
----------------
ruiu wrote:
> More explicit code would be easier to understand.
> 
>   // We tentatively added all synthetic sections at the beginning and removed
>   // empty ones afterwards (because there is no way to know whether they were
>   // going be empty or not other than actually running linker scripts.)
>   // We need to ignore remains of empty sections.
>   if (auto *Sec = SyntheticSection<ELFT>(ID))
>     if (Sec->empty())
>       continue;
Done.


================
Comment at: ELF/Writer.cpp:256
 
-  if (Config->EhFrameHdr)
-    In<ELFT>::EhFrameHdr = make<EhFrameHeader<ELFT>>();
-
-  if (Config->GdbIndex)
-    In<ELFT>::GdbIndex = make<GdbIndexSection<ELFT>>();
+  // Initialize linker generated sections
+  if (!Config->Relocatable)
----------------
ruiu wrote:
> Remove this comment because throughout this function we initialize linker generated sections.
Done.


================
Comment at: ELF/Writer.cpp:340
+  // We always need to add rel[a].plt to output if it has entries.
+  // Even during static linking it can contain R_[*]_IRELATIVE relocations.
+  In<ELFT>::RelaPlt = make<RelocationSection<ELFT>>(
----------------
ruiu wrote:
> Even for ...
Done.


================
Comment at: ELF/Writer.cpp:922-923
         std::find(OutSec->Sections.begin(), OutSec->Sections.end(), SS));
+    // Mark section as unused to stop finalizing it and proccessing from script.
+    SS->OutSec = nullptr;
     // If there is no other sections in output section, remove it from output.
----------------
ruiu wrote:
> I don't think using this as a marker is a good idea. Please read the above comment.
Removed.


================
Comment at: ELF/Writer.cpp:1025-1027
+  addInputSec(In<ELFT>::SymTab);
+  addInputSec(In<ELFT>::ShStrTab);
+  addInputSec(In<ELFT>::StrTab);
----------------
ruiu wrote:
> I do not understand why only these sections are handled in this function. Is there any reason you can't add them in createSyntheticSections?
When I tried to move these ones, I ended up with "llvm-objdump.EXE: error reading file: Invalid data was encountered while parsing the file." in sections.s testcase.
I investigated it:

Issue comes from readobj:

```
template <class ELFT>
Expected<StringRef>
ELFFile<ELFT>::getStringTable(const Elf_Shdr *Section) const {
  if (Section->sh_type != ELF::SHT_STRTAB)
    return createError("invalid sh_type for string table, expected SHT_STRTAB");
```

Section->sh_type for  .shstrtab is not valid here.
Test itself declares the .shstrtab section. That triggers an issue.
```
.section .shstrtab,""
.short 20
```

Previously we had 2 .shstrtab sections in output for that test:

```
Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
.....
  [ 5] .shstrtab         PROGBITS         0000000000000000  00001031
       0000000000000002  0000000000000000           0     0     1
  [ 6] .comment          PROGBITS         0000000000000000  00001033
       0000000000000012  0000000000000001  MS       0     0     1
  [ 7] .symtab           SYMTAB           0000000000000000  00001048
       0000000000000030  0000000000000018           9     1     8
  [ 8] .shstrtab         STRTAB           0000000000000000  00001078
       000000000000003b  0000000000000000           0     0     1
  [ 9] .strtab           STRTAB           0000000000000000  000010b3
       0000000000000008  0000000000000000           0     0     1

```

But if after adding there 3 to Symtab<ELFT>::X->Sections we end up with a single .shstrtab in output that
has 2 sections inside (one regular and one synthetic). 
Sections header becomes invalid as result section is not SHT_STRTAB.
I am not sure why we might want to support non allocatable sections for script processing.
But anyways even if we want to do that, that looks to be a different patch that also should fix issue above.





https://reviews.llvm.org/D27131