This is an archive of the discontinued LLVM Phabricator instance.

Split the creation of program headers in a few steps
ClosedPublic

Authored by rafael on Feb 8 2016, 8:30 AM.

Details

Reviewers
ruiu
grimar
Summary

IMHO this makes the code easier to read and should help with linker scripts.

This is strongly based on D16575. The main differences are

  • We record a range of sections, not every section in a program header.
  • ScanHeaders takes case of deciding what goes in every program header, including PT_GNU_RELRO
  • We create dummy sections for the start of the file

With this, program header creation has 3 isolated stages:

  1. Map sections to program headers.
  2. Assign addresses to *sections*
  3. Looking at sections find the address and size of each program header.

Diff Detail

Event Timeline

rafael updated this revision to Diff 47208.Feb 8 2016, 8:30 AM
rafael retitled this revision from to Split the creation of program headers in a few steps.
rafael updated this object.
rafael added reviewers: ruiu, grimar.
rafael set the repository for this revision to rL LLVM.
emaste added a subscriber: emaste.Feb 8 2016, 9:45 AM
emaste added inline comments.
ELF/Writer.cpp
45

extra 'm'

grimar edited edge metadata.Feb 9 2016, 1:05 AM

That looks better then my approach I think, thanks ! Have a consern about dummies output sections thought.

ELF/Writer.cpp
96

"create"

103

Honestly, I dont like the approach to have dummies like that. Thats looks like a hack (more like a hack in compare with 'special' first load).
Linkerscript allows to put elf header and phdr anywhere afaik. But the question is do we really need to support that ?
In my patch about PHDRS (http://reviews.llvm.org/D16771) I introduced the restriction about that:
first phdr is always PT_PHDR and first load must contain elf and phdr headers. It seems your implementation is really close or the same to mine here (at least it assumes them are at start always), but do we really need to have dummies logic then ?
IMO special first load and less entities looks better here.

186

Let's place them before all other sections as they are usually always go before all other ones ?

Honestly, I dont like the approach to have dummies like that. Thats looks like a hack (more like a hack in compare with 'special' first load).
Linkerscript allows to put elf header and phdr anywhere afaik. But the question is do we really need to support that ?
In my patch about PHDRS (http://reviews.llvm.org/D16771) I introduced the restriction about that:
first phdr is always PT_PHDR and first load must contain elf and phdr headers. It seems your implementation is really close or the same to mine here (at least it assumes them are at start always), but do we really need to have dummies logic then ?
IMO special first load and less entities looks better here.

I remember seeing that used in a case where it was desired that the
elf and program headers were not part of a PT_LOAD (NaCl). It is
unclear if it needs to be supported, but if it is an "Inventor's
paradox" situation, why not support the more general case?

The problem with having PT_LOAD that is before any section is that we
need special cases in two places:

  • The first section in it doesn't need to be page aligned, since it is

not the start of the pt_load.

  • Computing its actual position.

Not having a section for the program headers also means PT_PHDR is a
special case. Implementing this patch is how I found the bug I fixed
in r260102.

Comment at: ELF/Writer.cpp:186
@@ -162,2 +185,3 @@
+ Out<ELFT>::ProgramHeaders = &ProgramHeaders;

Writer<ELFT>(*Symtab).run();

Let's place them before all other sections as they are usually always go before all other ones ?

Not sure I follow. They are already at the start.

I will upload a rebased/fixed patch.

Cheers,
Rafael

rafael updated this revision to Diff 47350.Feb 9 2016, 11:59 AM
rafael edited edge metadata.
rafael removed rL LLVM as the repository for this revision.

Rebased and comment fixes.

ruiu added inline comments.Feb 9 2016, 4:12 PM
ELF/Writer.cpp
53

You can "Elf_Phdr H = {}" here, can't you?

54–55

I think I like this idea. I was thinking to make Phdr a list of output sections, but that was too flexible. This two-pointer approach is on the other hand directly reflects what we actually have in an ELF file.

95

s/a a/a/
s/elf/ELF/

96

I think we usually omit int.

137

Move this to beginning of the function.

1198–1200

You may want to return a pointer instead of a reference because in a few places in this function you wrote "&AddHdr".

1253

// Add the TLS segment unless it's empty.

1284–1285

I think this function call worth to be moved to Writer::run because now PHDRs construction and address assignments are nicely separated. You may want to rename scanHeaders to createPhdrs or something like that.

1294–1295

Why don't you use a range-based for loop?

1296–1302

Can you add a comment for this piece of code? It is not obvious.

1324

Why process only needsPhdr sections?

1353

I wouldn't use auto here.

1446

Instead of uint8_t *, you can cast to Elf_Phdr *.

grimar added inline comments.Feb 10 2016, 2:14 AM
ELF/Writer.cpp
1198–1200

I agree here, that also eliminates the need of direct specify of return type for functor (it usually doesn't look good, here as well).

1296–1302

If you're not using range-loop (but I think it would be ok here),
could you not use the variable name that replaces the name in global scope ? I mean you already have "I" above.

Comment at: ELF/Writer.cpp:1324
@@ +1323,3 @@
+
+ if (needsPhdr<ELFT>(Sec)) {

+ // Don't allocate VA space for TLS NOBITS sections. The PT_TLS PHDR is

Why process only needsPhdr sections?

Other sections don't have a VA, which is what we set in the if body.

I will upload a patch with the other suggestions.

Cheers,
Rafael

rafael updated this revision to Diff 47450.Feb 10 2016, 7:17 AM
rafael set the repository for this revision to rL LLVM.

Address Rui's and George's comments.

grimar accepted this revision.Feb 10 2016, 7:47 AM
grimar edited edge metadata.

LGTM. But that is just mine opinion, lets wait to see Rui's one. Btw, thanks, that looks better than mine version I think !

This revision is now accepted and ready to land.Feb 10 2016, 7:47 AM
ruiu accepted this revision.Feb 10 2016, 11:12 AM
ruiu edited edge metadata.

LGTM

ELF/Writer.cpp
1259–1260

nit: Can it fit in 80 cols if you rename H?

1324

Please leave a comment that sections that are not in any PHDRs don't have VA (because they are not mapped to anywhere at runtime).

1447–1450

I think you can do

for (Phdr & P : Phdrs)
  *HBuf++ = P.H;
grimar closed this revision.Feb 17 2016, 1:34 AM

Was commited as r260453