Page MenuHomePhabricator

[llvm-objcopy]Allow llvm-objcopy to be used on an ELF file with no section headers
ClosedPublic

Authored by jhenderson on Mar 29 2019, 6:11 AM.

Details

Summary

This patch fixes https://bugs.llvm.org/show_bug.cgi?id=41293 and https://bugs.llvm.org/show_bug.cgi?id=41045. llvm-objcopy assumed that it could always read a section header string table. This isn't the case when the sections were previously all stripped, and the e_shstrndx field was set to 0. This patch fixes this. It also fixes a double space in an error message relating to this issue, and prevents llvm-objcopy from adding extra space for non-existent section headers, meaning that --strip-sections on the output of a previous --strip-sections run produces identical output, simplifying the test.

Diff Detail

Repository
rL LLVM

Event Timeline

jhenderson created this revision.Mar 29 2019, 6:11 AM
Herald added a project: Restricted Project. · View Herald Transcript

Few nits/questions from me.

test/tools/llvm-objcopy/ELF/copy-after-strip-sections.test
23 ↗(On Diff #192812)

Is this field important?

28 ↗(On Diff #192812)

Do you need PAddr for this test?

tools/llvm-objcopy/ELF/Object.cpp
1573 ↗(On Diff #192812)

size is a bit strange helper. I do not think it is common to use it instead of .size() call
for a container in LLVM..
I see it is not your initial code, but since you're changing it, what do you think about not using it here at all?

tools/llvm-objcopy/ELF/Object.h
233 ↗(On Diff #192812)

Up to you perhaps, but I would suggest to move it out to the constructor body.

jhenderson marked 2 inline comments as done.Mar 29 2019, 7:10 AM
jhenderson added inline comments.
test/tools/llvm-objcopy/ELF/copy-after-strip-sections.test
23 ↗(On Diff #192812)

Not massively, no. But it does force the section to appear at offset 0x1000. I was originally going to check the contents, but I decided not to do that, so I can get rid of it and the Address below.

28 ↗(On Diff #192812)

It's not important. Most of our tests just have both. I can remove it and VAddr too, given the other changes.

jhenderson marked an inline comment as done.Mar 29 2019, 7:32 AM
jhenderson added inline comments.
tools/llvm-objcopy/ELF/Object.cpp
1573 ↗(On Diff #192812)

The underlying container doesn't currently have a size() method. There are quite a few places in this file that seem to use this helper, so perhaps it should be a different change?

grimar added inline comments.Mar 29 2019, 7:58 AM
tools/llvm-objcopy/ELF/Object.cpp
1573 ↗(On Diff #192812)

Oh, I thought it has. I have no objections then.

Address review comments.

grimar accepted this revision.Mar 29 2019, 8:39 AM

LGTM.

This revision is now accepted and ready to land.Mar 29 2019, 8:39 AM
rupprecht accepted this revision.Mar 29 2019, 12:11 PM
rupprecht added inline comments.
tools/llvm-objcopy/ELF/Object.h
234 ↗(On Diff #192834)

Slightly simpler:

ELFWriter(Object &Obj, Buffer &Buf, bool WSH)
    : Writer(Obj, Buf), WriteSectionHeaders(WSH && Obj.HadShdrs) {}
jhenderson updated this revision to Diff 193266.Apr 2 2019, 6:13 AM

Rebase and address comment.

Unfortunately, this build fails on Linux. Somehow MSVC is able to handle the incomplete type of Object in the ELFWriter constructor, whereas the version of GCC that I'm using doesn't. The Writer classes aren't really part of the Object interface, so I'm going to move them into a separate header.

jhenderson updated this revision to Diff 193280.Apr 2 2019, 6:59 AM

Move constructor definition into .cpp file to avoid dependency on Object issues.

grimar accepted this revision.Apr 2 2019, 7:01 AM

LGTM

This revision was automatically updated to reflect the committed changes.