This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by grimar on Nov 25 2016, 7:00 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar updated this revision to Diff 79302.Nov 25 2016, 7:00 AM
grimar retitled this revision from to [ELF] - Add support of proccessing of the rest allocatable synthetic sections from linkerscript..
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar, evgeny777.
evgeny777 added inline comments.Nov 25 2016, 8:05 AM
test/ELF/linkerscript/phdrs.s
98 ↗(On Diff #79302)

What's this change for?

emaste added a subscriber: emaste.Nov 25 2016, 8:21 AM
emaste added inline comments.
test/ELF/eh-frame-marker.s
6–7 ↗(On Diff #79302)

I don't think it should matter, but having these two sections ordered this way is consistent with what the GNU linkers do.

12–15 ↗(On Diff #79302)

Interesting, I learned something new about lit!

grimar added a comment.EditedNov 27 2016, 11:58 PM
test/ELF/eh-frame-marker.s
6–7 ↗(On Diff #79302)

I did not try to check their relative order here. I think it is not what is important.
Problem is that line

// CHECK:      Name: .eh_frame

accepts foth .eh_frame and .eh_frame_hdr. I did not find the way to do exact matching. So first line here is just to skip the hdr section.

test/ELF/linkerscript/phdrs.s
98 ↗(On Diff #79302)

Testcase has next script:

SECTIONS { . = SIZEOF_HEADERS; .foo : { *(.*) } : text : foo}
That means it puts all sections to foo, and since this patch added few more synthetic sections available from script,
it put it there and total size grew up.

ruiu added inline comments.Nov 28 2016, 5:17 PM
ELF/LinkerScript.cpp
484–485 ↗(On Diff #79302)

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;
ELF/Writer.cpp
256 ↗(On Diff #79302)

Remove this comment because throughout this function we initialize linker generated sections.

340 ↗(On Diff #79302)

Even for ...

922–923 ↗(On Diff #79302)

I don't think using this as a marker is a good idea. Please read the above comment.

1025–1027 ↗(On Diff #79302)

I do not understand why only these sections are handled in this function. Is there any reason you can't add them in createSyntheticSections?

grimar updated this revision to Diff 79570.Nov 29 2016, 7:25 AM
  • Addressed review comments.
ELF/LinkerScript.cpp
484–485 ↗(On Diff #79302)

Done.

ELF/Writer.cpp
256 ↗(On Diff #79302)

Done.

340 ↗(On Diff #79302)

Done.

922–923 ↗(On Diff #79302)

Removed.

1025–1027 ↗(On Diff #79302)

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.

ruiu accepted this revision.Nov 29 2016, 7:57 AM
ruiu edited edge metadata.

LGTM

This revision is now accepted and ready to land.Nov 29 2016, 7:57 AM
This revision was automatically updated to reflect the committed changes.