Page MenuHomePhabricator

[ELF] - implemented --eh-frame-hdr command line option.
ClosedPublic

Authored by grimar on Dec 22 2015, 4:37 AM.

Details

Summary

--eh-frame-hdr
Request creation of ".eh_frame_hdr" section and ELF "PT_GNU_EH_FRAME" segment header.

Both gold and the GNU linker support an option --eh-frame-hdr which tell them to construct a header for all the .eh_frame sections. This header is placed in a section named .eh_frame_hdr and also in a PT_GNU_EH_FRAME segment. At runtime the unwinder can find all the PT_GNU_EH_FRAME segments by calling dl_iterate_phdr.
This section contains a lookup table for quick binary search of FDEs.
Detailed info can be found here:
http://www.airs.com/blog/archives/462

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ruiu added inline comments.Dec 24 2015, 9:39 PM
ELF/OutputSections.cpp
767 ↗(On Diff #43546)

Please add a short comment with the URL to the Ian's blog article.

771 ↗(On Diff #43546)

Is this value correct? (If correct, it needs a comment.)

774 ↗(On Diff #43546)

This function needs a brief comment.

783–791 ↗(On Diff #43546)

I'd make this a function so that you write

case ...:
  return read16<E>(F.PCRel);

instead of

case ...:
  PcRel = read16<E>(F.PCRel);
  break;
829 ↗(On Diff #43546)

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

860 ↗(On Diff #43546)

Ah, OK, so sh_size is initialized to 12 and change in size as you add new FDEs. You want to write about that as a comment.

1032 ↗(On Diff #43546)

Add a comment.

// Read a byte and advance D by one byte.

I think you don't need to template this function.

1133 ↗(On Diff #43546)

You can remove template <class ELFT>. Please move this just below readByte.

1183 ↗(On Diff #43546)

Isn't this a fatal error?

ELF/OutputSections.h
450 ↗(On Diff #43546)

Alive -> Live.

test/ELF/eh-frame-hdr-no-out.s
2 ↗(On Diff #43546)

How did you create the binary file? Can you use an assembler to create that file?

grimar updated this revision to Diff 43624.Dec 25 2015, 3:07 AM

Review comments addressed.

ELF/OutputSections.cpp
767 ↗(On Diff #43546)

Done.

771 ↗(On Diff #43546)

Added comment.

774 ↗(On Diff #43546)

Done.

829 ↗(On Diff #43546)

Interesting question. Fixed.

860 ↗(On Diff #43546)

Added comment.

1032 ↗(On Diff #43546)

Done.

1133 ↗(On Diff #43546)

Ah, yes, initially I did it as class member and forgot to remove after making static.
Fixed.

1183 ↗(On Diff #43546)

Thats a question. I think it is not. Linker here just should merge the .eh_frame sections in the way it can. It does not need to support any new versions for example to do that. But .eh_frame_hdr can`t be generated then without knowing internals.
Since my patch is about generating eh_frame_hdr then its correct for it to be Live=false with warning I think.

test/ELF/eh-frame-hdr-no-out.s
2 ↗(On Diff #43546)

I dont know any way to generate corrupted CIE or CIE with specified version using assembler.
That is done by compiler itself I believe.
So I just took sample and then used hex editor to manually change the version.

grimar updated this revision to Diff 43625.Dec 25 2015, 3:12 AM

Fixed urls formatting.

ruiu added inline comments.Jan 8 2016, 2:55 PM
ELF/OutputSections.cpp
767 ↗(On Diff #43625)

Add a space before "--".

778–779 ↗(On Diff #43625)

Remove "llvm::ELF::".

780 ↗(On Diff #43625)

` -> '

781 ↗(On Diff #43625)

Its -> It's

794 ↗(On Diff #43625)

Can you remove "typename EhFrameHdr<ELFT>::"?

800 ↗(On Diff #43625)

switch (F.enc & 0xF)

831 ↗(On Diff #43625)

It is better to advance Buf here.

Buf += 12;
851 ↗(On Diff #43625)

Isn't parameter Sec be non-null? If it is, you can just do

if (this->Sec != Sec)
1122–1124 ↗(On Diff #43625)

We can exit from this function as soon as we read data, no?

return readByte(D, &Cie.FdeEncoding));
1136 ↗(On Diff #43625)

Ditto

ELF/OutputSections.h
436 ↗(On Diff #43625)

Please add a comment to this class.

437 ↗(On Diff #43625)

You can remove this typedef.

grimar updated this revision to Diff 44511.Jan 11 2016, 7:57 AM
grimar marked 9 inline comments as done.
  • Addressed review comments.
  • Rebased.
ELF/OutputSections.cpp
767 ↗(On Diff #43625)

Done, comment was moved to class EhFrameHdr definition.

780 ↗(On Diff #43625)

Removed that line.

851 ↗(On Diff #43625)

Point to do "if (this->Sec)" check here is to avoid warning and disabling of header for first assignEhFrame call. I only want to warn if one ehframe is assigned already and somebody tries to assign another one.

1122–1124 ↗(On Diff #43625)

I am not sure it is better. At first look it`s like a little optimization of parsing augmentation string but in fact it can hide a potential error when CIE information is corrupted. Early exit can hide warning about that if 'R' is not the latest in a sequence.

1136 ↗(On Diff #43625)

Hmm, I dont think its "Ditto", the propose of the function is to read 1 byte that represents the pointer encoding for the address pointers used in the FDE, which is read in 'R', so we need to read and skip everything we met at least until we read 'R'.

ruiu added inline comments.Jan 11 2016, 10:38 AM
ELF/OutputSections.cpp
764 ↗(On Diff #44511)

` -> '

777 ↗(On Diff #44511)

Remove this empty line.

844 ↗(On Diff #44511)

So, if Sec is nonnull but this->Sec can be null,

  • If this->Sec == nullptr, (this->Sec != Sec) can never be true (because Sec is not null), no warning is displayed.
  • If this->Sec != nullptr, (this->Sec != Sec) is true only when somebody tries to assign another one.

But this is a minor point. Leave it as is if you think yours more readable.

1111–1113 ↗(On Diff #44511)

I'm pretty sure that that is better if this code is just for checking consistency. We generally don't want to read any data unless it is absolutely necessary to link stuff. It is compiler's responsibility to feed consistent file to the linker, so the linker doesn't have to verify all bits. Doing less is important for performance.

grimar updated this revision to Diff 44614.Jan 12 2016, 2:26 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
ELF/OutputSections.cpp
764 ↗(On Diff #44511)

Sorry, I dont get what you want me to do here ?

844 ↗(On Diff #44511)
if (this->Sec != Sec) {
 warning("multiple .eh_frame sections not supported for .eh_frame_hdr");
...

So initially (this->Sec != Sec) is always true, because this->Sec is null and Sec is not. Them are always not equal at first run. Warning is displayed, but it should not.

1111–1113 ↗(On Diff #44511)

Ok, done.

ruiu added inline comments.Jan 12 2016, 5:37 AM
ELF/OutputSections.cpp
764 ↗(On Diff #44614)

I meant please use apostrophe (') instead of backquote (`) for contractions.

1073 ↗(On Diff #44614)

This function reads only encoding, right? Then maybe we should change this function signature to

ErrorOr<uint8_t> getFdeEncoding(ArrayRef<uint8_t> D)

?

Also I'm wondering if corrupted CIEs/FDEs are so prevalent that we have to take care of them. For now, I'd like to simply use error() whenever we find something wrong. Then we can change the signature to

uint8_t getFdeEncoding(ArrayRef<uint8_t> D)

which is pretty simple to understand.

grimar updated this revision to Diff 44645.Jan 12 2016, 9:30 AM
grimar marked 2 inline comments as done.

Addressed review comments (refactored getFdeEnc method).

ELF/OutputSections.cpp
1073 ↗(On Diff #44614)

What about getFdeEnc() then ? (to calm down the clang format).
I also changed readByte and skipLeb128 to uint8_t readByte() and void skipLeb128().
And introduced cieParseError() more method to report CIE parse errors in a single place.

grimar added inline comments.Jan 12 2016, 9:32 AM
ELF/OutputSections.cpp
1073 ↗(On Diff #44645)

Please ignore getFdeEnc part. getFdeEncoding() is fine now, I`ll update patch in a minute.

grimar updated this revision to Diff 44647.Jan 12 2016, 9:33 AM

getFdeEnc() -> getFdeEncoding()

ruiu added inline comments.Jan 12 2016, 11:04 AM
ELF/OutputSections.cpp
844 ↗(On Diff #44647)

You can remove {}.

1008–1009 ↗(On Diff #44647)

Please use Type and Flags instead of sh_type and sh_flags because of the LLVM coding style.

1031–1033 ↗(On Diff #44647)

Do we need this? If this function is called only at a few places, I'd just inline it.

1059 ↗(On Diff #44647)

This should be error() instead of llvm_reachable() because it is logically reachable if an input is corrupted.

1086 ↗(On Diff #44647)

This is probably better.

if (Version != 1 && Version != 3)
  error("FDE version 1 or 3 expected, but got " + Version);
1094 ↗(On Diff #44647)
if (readByte(D) != 1)
  error(".eh_frame alignment must be 1")
1100–1105 ↗(On Diff #44647)

At this point it is guaranteed that Version is either 1 or 3, so

if (Version == 1)
  readByte(D);
else
  skipLeb128(D);
1117 ↗(On Diff #44647)

More specific error message would be appreciated.

1130 ↗(On Diff #44647)

More specific error message would be good. (error("Unknown XXX") or something like that.)

ELF/OutputSections.h
288 ↗(On Diff #44647)

Is unsigned the right type here? If the linker runs on a 32-bit computer, it is 32-bit, but can it link a large 64-bit program?

444 ↗(On Diff #44647)

Replace ` with ' (in general for propositions.)

grimar updated this revision to Diff 44718.Jan 13 2016, 1:51 AM
grimar marked 9 inline comments as done.
  • Addressed review comments
  • Fixed eh-frame-hdr-no-out.s test
ELF/OutputSections.cpp
1008–1009 ↗(On Diff #44647)

Hmm not sure how that happend. Fixed.

1031–1033 ↗(On Diff #44647)

Now when there are few more specific errors introduced there is probably no need in that.

1059 ↗(On Diff #44647)

Yes, nasty copypast issue :( I think I took it from llvm\tools\llvm-objdump\MachODump.cpp or llvm\lib\MC\MCDwarf.cpp where getSizeForEncoding() have the same one..

1094 ↗(On Diff #44647)

Changed message to "CIE code alignment must be 1". As CIE also has data alignment factor.

ELF/OutputSections.h
288 ↗(On Diff #44647)

Its constructed from
unsigned Index = S->Offsets.size();
...
Cie<ELFT> C(S, Index);
So it seems more correct would be to use size_t instead of unsigned. But I guess its not possible in real life to have overflow here (its hard to imagine).

ruiu accepted this revision.Jan 13 2016, 12:22 PM
ruiu edited edge metadata.

LGTM with a few nits.

ELF/OutputSections.cpp
814–815 ↗(On Diff #44718)

Write this in one line.

822 ↗(On Diff #44718)

It is odd that we are using uintX_t here but we write it as a 32-bit integer. Shouldn't we just use uint32_t here?

825 ↗(On Diff #44718)

Ditto

1051 ↗(On Diff #44718)

format -> Format

1099 ↗(On Diff #44718)
else if (Version == 3) -> else

because we know that it's always 3.

ELF/OutputSections.h
438 ↗(On Diff #44718)

Please move this above the definition of align.

This revision is now accepted and ready to land.Jan 13 2016, 12:22 PM
grimar added inline comments.Jan 13 2016, 2:39 PM
ELF/OutputSections.cpp
825 ↗(On Diff #44718)

What about eliminating them ?

  for (auto &I : PcToOffset) {
    // The first four bytes are an offset to the initial PC value for the FDE.
    write32<E>(Buf, I.first - VA);
    // The last four bytes are an offset to the FDE data itself.
    write32<E>(Buf + 4, EhVA + I.second - VA);
    Buf += 8;
  }
}
ruiu added inline comments.Jan 13 2016, 2:40 PM
ELF/OutputSections.cpp
825 ↗(On Diff #44718)

LGTM

grimar added inline comments.Jan 13 2016, 2:50 PM
ELF/OutputSections.cpp
825 ↗(On Diff #44718)

Nice. Thanks for review ! I'll commit this tomorrow.

This revision was automatically updated to reflect the committed changes.
grimar marked 9 inline comments as done.
grimar added inline comments.Jan 14 2016, 2:34 AM
ELF/OutputSections.cpp
1051 ↗(On Diff #44718)

Replaced with

switch (Enc & 0x0f) {
ELF/OutputSections.h
481 ↗(On Diff #44718)

I renamed class EhFrameHdr to EhFrameHeader because of compile error under linux here.