This is an archive of the discontinued LLVM Phabricator instance.

ELF: Write .eh_frame_hdr explicitly after writing .eh_frame.
ClosedPublic

Authored by pcc on Feb 28 2019, 2:13 PM.

Details

Summary

This lets us remove the special case from Writer::writeSections(), and also
fixes a bug where .eh_frame_hdr isn't necessarily written in the correct
order if a linker script moves .eh_frame and .eh_frame_hdr into the same
output section.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

pcc created this revision.Feb 28 2019, 2:13 PM
Herald added a project: Restricted Project. · View Herald Transcript
ruiu added inline comments.Feb 28 2019, 2:24 PM
lld/ELF/OutputSections.h
128 ↗(On Diff #188788)

At first I thought that this is a base address or something, but turned out this is just a pointer to the beginning of a buffer. Maybe BufferStart is a better name?

pcc updated this revision to Diff 188797.Feb 28 2019, 2:57 PM

Address review comment

pcc marked an inline comment as done.Feb 28 2019, 2:58 PM
pcc added inline comments.
lld/ELF/OutputSections.h
128 ↗(On Diff #188788)

Makes sense, done.

ruiu accepted this revision.Feb 28 2019, 2:58 PM

LGTM

This revision is now accepted and ready to land.Feb 28 2019, 2:58 PM
This revision was automatically updated to reflect the committed changes.

It looks like this change broke the clang-armv7-linux-build-cache which uses clang 5.0.1 running on a 32-bit ARM container http://lab.llvm.org:8011/builders/clang-armv7-linux-build-cache/builds/11063 . I've committed a temporary fix that r355195 that changes FileSize to use size_t rather than uint64_t. Mentioning it here just in case you want to fix it in different way.

pcc added a comment.Mar 1 2019, 10:40 AM

It looks like this change broke the clang-armv7-linux-build-cache which uses clang 5.0.1 running on a 32-bit ARM container http://lab.llvm.org:8011/builders/clang-armv7-linux-build-cache/builds/11063 . I've committed a temporary fix that r355195 that changes FileSize to use size_t rather than uint64_t. Mentioning it here just in case you want to fix it in different way.

Hmm, it should probably be a uint64_t so that we can detect size overflows better on 32-bit platforms. Sent D58840 with the fix.