This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][ELF] Implement a mutable section visitor that updates size-related fields (Size, EntrySize, Align) before layout.
ClosedPublic

Authored by rupprecht on Jan 2 2019, 11:43 AM.

Details

Summary

Fix EntrySize, Size, and Align before doing layout calculation.

As a side cleanup, this removes a dependence on sizeof(Elf_Sym) within BinaryReader, so we can untemplatize that.

This unblocks a cleaner implementation of handling the -O<format> flag. See D53667 for a previous attempt. Actual implementation of the -O<format> flag will come in an upcoming commit, this is largely a NFC (although not _totally_ one, because alignment on binary input was actually wrong before).

Diff Detail

Repository
rL LLVM

Event Timeline

rupprecht created this revision.Jan 2 2019, 11:43 AM

This is a preety nice solution. I'd been working on removing a lot of the fields from SectionBase, and making an object that had all the sizes of various things listed in it, and constructing that from the ELFT and moving all the calculation of such things into the Writer. That changes kept getting larger and more hairy. This is still quite clean and far less invasive. Also I think a mutable section visitor is a generically useful thing to have around. Nice work. I'd wait for one more LGTM. Sorry I wasn't able to get the change I promised out in time.

jhenderson accepted this revision.Jan 3 2019, 2:18 AM

LGTM, with a couple of minor comments.

I am somewhat concerned that we're adding another loop over all the sections, as this could be somewhat slow for very large objects, but it's probably a silly concern. However, it might be nice to move the section sizing later, to the point where all sections have been added, into the Index setting loop. Otherwise, when we are adding any additional sections we have to ensure that their sizes are correct, whereas the section sizer could do that all for us. I don't mind really though.

tools/llvm-objcopy/ELF/Object.cpp
89 ↗(On Diff #179906)

I assume here and below, sh_addralign is used as a placeholder for "whatever is the largest data field in the struct" (i.e. sh_size or whatever would be equally valid)?

tools/llvm-objcopy/ELF/Object.h
91 ↗(On Diff #179906)

I'm a little troubled with how similar this is to SectionVisitor, but I don't have a better solution overall. However, I would like it for the destructors to be defined in the same manner. At the moment this one is defaulted, and the other is defined out-of-line.

156–157 ↗(On Diff #179906)

I'm not sure you need either of these? The class has no members, so these should be trivially generated by the compiler with no need to be explicit. I think (I might be misremembering my C++...)

This revision is now accepted and ready to land.Jan 3 2019, 2:18 AM
rupprecht updated this revision to Diff 180091.Jan 3 2019, 9:42 AM
rupprecht marked 3 inline comments as done.
  • Merge loops over sections
  • Clean up constructors
  • Explicitly use the largest field in the struct for addralign
rupprecht added inline comments.Jan 3 2019, 9:43 AM
tools/llvm-objcopy/ELF/Object.cpp
89 ↗(On Diff #179906)

Yes, I'm pretty sure that's where this is coming from. It's been a bit since I first wrote this patch though, so I'll have to do some digging today -- this matches what GNU objcopy seems to do, but there may be a more technically correct way to do this.

This revision was automatically updated to reflect the committed changes.
jhenderson added inline comments.Jan 3 2019, 9:51 AM
llvm/trunk/tools/llvm-objcopy/ELF/Object.cpp
89

Nit: larget -> largest

97

Ditto.

rupprecht marked an inline comment as done.Jan 3 2019, 9:56 AM
rupprecht added inline comments.
llvm/trunk/tools/llvm-objcopy/ELF/Object.cpp
89

d'oh.... haven't had coffee yet :(

Fixed in r350337