This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][MachO] Recompute and update offset/size fields in the writer
ClosedPublic

Authored by seiya on May 30 2019, 5:03 AM.

Details

Summary

Recompute and update offset/size fields so that we can implement llvm-objcopy options like --only-section.

This patch is the first step and focuses on supporting load commands that covered by existing tests: executable files and
dynamic libraries are not supported.

Diff Detail

Event Timeline

seiya created this revision.May 30 2019, 5:03 AM

great start! I will take a closer look at the diff tomorrow + over the weekend

compnerd added inline comments.
llvm/test/tools/llvm-objcopy/MachO/Inputs/various-symbols.s
3

NIT: common is misspelt

5

global3 is unreferenced. Is that intended?

llvm/test/tools/llvm-objcopy/MachO/real-world-input-copy.test
13

The test input seems oddly complicated for testing that. It seems that anything that contains a LC_DYNSYMTAB which means anything with an externally visible symbol should be sufficient. The BSS section would be emitted by any static. Something like this seems much more concise which is better for testing the cases:

static int i;
int f(void) { return i; }
llvm/tools/llvm-objcopy/MachO/MachOReader.cpp
22

Isn't this just strnlen?

34

I believe that the 16 here is supposed to be the maximum length of the section/segment name in MachO. Can you use sizeof(llvm::MachO::section::sectname) and sizeof(llvm::MachO::section::segname) instead please?

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp
139

I wish that we had the structs built using packed_endian types

143

Can the Sec be const?

146

I think that you can drop copyStringWithPadding. Instead zero initialise the section that you are constructing, which also gives you more reproducible output.

462

Can't Sec be const?

470

VM is more appropriate - it is an initialism for Virtual Memory.

501

Can't LC be const?

502

Can't Sec be const?

510

Can't LC be const?

llvm/tools/llvm-objcopy/MachO/MachOWriter.h
32

Might be more readable as updateDySymTab

llvm/test/tools/llvm-objcopy/MachO/real-world-input-copy.test
13

+1

seiya updated this revision to Diff 202344.May 30 2019, 6:12 PM
  • Simplified various-symbols.s.
  • Replaced strlenOrMaxLen (introduced by this patch) with strnlen.
  • Replaced the hard-coded lengths of segment/section name with sizeof().
  • Renamed updateDysymtab to updateDySymTab .
  • Replaced copyStringWithPadding (introduced by this patch) with memset-ing by zero and memcpy.
  • Add const to a variable.
seiya marked 22 inline comments as done.May 30 2019, 6:30 PM
seiya added inline comments.
llvm/test/tools/llvm-objcopy/MachO/real-world-input-copy.test
13

Indeed. Simplified the test case.

llvm/tools/llvm-objcopy/MachO/MachOReader.cpp
22

Quite so :)

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp
139

I wonder if we could do this better too. Could you tell me what packed_endian is? You mean llvm/Support/Endian.h? I'd like to take a look at it.

462

No. It's mutated in the loop.

471

I've replaced the calculation of VMSize by the algorithm used in MachObjectWriter::writeObject

501

It can't be const since its sections are mutated in the loop.

502

No. It's mutated in the loop.

510

No. LC. MachOLoadCommand is mutated in the loop.

seiya marked 6 inline comments as done.May 30 2019, 6:31 PM
seiya marked an inline comment as done.May 30 2019, 6:33 PM
seiya added inline comments.
llvm/test/tools/llvm-objcopy/MachO/Inputs/various-symbols.s
5

That was a mistake. I've updated this file so it no longer matters btw.

seiya updated this revision to Diff 202400.May 31 2019, 2:19 AM
  • add some consts
llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp
135

nit: I'd use the explicit type here (for readability)

147

khm, here and in some other similar places, i'd use sizeof(...)
or create a named constant for this number (16).

174

here and the lines above (143 - 170) are very similar,
let's create a helper function for this and avoid code duplication.

422

explicit type + I think naming here doesn't follow our style guide)

432

see the comment on the line 438

465
   
1 << Sec.Align

P.S. pow works with floating point numbers, I think we don't really need these conversions here

seiya updated this revision to Diff 202842.Jun 3 2019, 7:38 PM

Address review comments.

seiya marked 5 inline comments as done.Jun 3 2019, 7:39 PM
seiya updated this revision to Diff 202845.Jun 3 2019, 7:41 PM

Address remaining review comments.

seiya marked an inline comment as done.Jun 3 2019, 7:42 PM

so in general this looks very promising to me,
however test coverage needs improvements imo, though given that at the moment we don't expose any non-trivial options here, we can add more tests incrementally. Btw - for DYSYMTAB, SYMTAB - is it possible to add YAML-based tests ? or yaml2obj's support is not sufficient for this ?

I'd also wait for @jhenderson and/or @rupprecht to take a look at this code as well.

This revision is now accepted and ready to land.Jun 4 2019, 7:00 PM

I'm struggling for time at the moment, and am not really familiar with MachO at all, so it would take me quite some time to review this. If @rupprecht is happy, I am happy.

seiya added a comment.Jun 5 2019, 5:43 PM
In D62652#1530262, @alexshap wrote:

so in general this looks very promising to me,
however test coverage needs improvements imo, though given that at the moment we don't expose any non-trivial options here, we can add more tests incrementally. Btw - for DYSYMTAB, SYMTAB - is it possible to add YAML-based tests ? or yaml2obj's support is not sufficient for this ?

I'd also wait for @jhenderson and/or @rupprecht to take a look at this code as well.

It seems that yaml2obj supports DYSYMTAB and SYMTAB. I'll add YAML-based tests in another patch which implements symbol table rebuilding.

@seiya, ok, i think if nobody comments on this diff in the coming days it's safe to proceed and commit it, I kinda don't want to block it and I'm looking forward to more tests in the follow-up patches .

rupprecht accepted this revision.Jun 6 2019, 1:19 PM

Also not a Mach-O expert, but I looked through it and what I can understand looks fine (+ it passes existing tests). More tests in a future patch are always welcome though.

seiya added a comment.Jun 6 2019, 4:22 PM

Thank you all. I'll commit this later.

This revision was automatically updated to reflect the committed changes.