This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Handle command-line option -sectcreate SEG SECT FILE
ClosedPublic

Authored by gkm on Aug 6 2020, 11:57 PM.

Details

Summary

Handle command-line option -sectcreate SEG SECT FILE, which inputs a binary blob from FILE into SEG,SECT

Diff Detail

Event Timeline

gkm created this revision.Aug 6 2020, 11:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2020, 11:57 PM
gkm requested review of this revision.Aug 6 2020, 11:57 PM
gkm retitled this revision from [lld-macho] Handle command-line option -sectcreate SEG SECT FILE, which inputs a binary blob from FILE as SEG,SECT to [lld-macho] Handle command-line option -sectcreate SEG SECT FILE.
gkm edited the summary of this revision. (Show Details)
int3 added inline comments.Aug 7 2020, 9:54 AM
lld/MachO/InputFiles.cpp
313

none of the other InputFile constructors modify inputSections directly; I think it would better fit the pattern if OpaqueFile instead inserted the isec in its subsections map (at an offset of zero)

lld/test/MachO/sectcreate.s
5

would be interesting to test multiple -sectcreate flags in sequence, e.g. checking that we lay out their sections in sequence as well. Also, does ld64 interleave them amongst the other object files on the command line, or do the created sections all appear at the end? Would be good to match that behavior

9–10

FileCheck does a substring match, so you could drop the hex bytes and just match against the ASCII

gkm marked 3 inline comments as done.Aug 7 2020, 11:46 AM
gkm added inline comments.
lld/MachO/InputFiles.cpp
313

Quite right. Fixed.

lld/test/MachO/sectcreate.s
5

ld64 does not interleave. It places them all at the end, after the input files. I augmented the test case.

gkm updated this revision to Diff 283972.Aug 7 2020, 11:52 AM

Amend according to review feedback

gkm updated this revision to Diff 283973.Aug 7 2020, 11:58 AM

Shuffle option sequence to demonstrate merging of discontiguous -sectcreate sections

int3 accepted this revision.Aug 7 2020, 12:03 PM

lgtm modulo those changes

lld/MachO/InputFiles.cpp
313

nit: subsections.emplace_back({0, isec});

lld/test/MachO/sectcreate.s
5

Thanks for checking! I think it's worthwhile for us to codify this behavior -- we could put %t.o in between the sectcreate flags below to demonstrate that interleaving doesn't occur.

12

let's check that __text appears before the additional sections

This revision is now accepted and ready to land.Aug 7 2020, 12:03 PM
gkm added inline comments.Aug 7 2020, 12:11 PM
lld/MachO/InputFiles.cpp
313

I followed prior practice in InputFile::parseSections. If I follow your recommendation, it seems I should also fix InputFile::parseSections, but then I am straying from the purpose of this diff into cleanup territory. Perhaps we should rather save cleanups for a separate diff, which could also include explicit const auto.

int3 added inline comments.Aug 7 2020, 12:16 PM
lld/MachO/InputFiles.cpp
313

I think it's worthwhile to make new code conform to the linter. That way, even if we don't explicitly do cleanups, we will still organically move to the new convention in the course of normal code churn from development. But I'm fine with migrating to emplace in a separate diff.

int3 added a reviewer: Restricted Project.Aug 7 2020, 12:18 PM
gkm added inline comments.Aug 7 2020, 12:39 PM
lld/MachO/InputFiles.cpp
313

I gave-up on subsections.emplace_back(). None of these compiled:

subsections.emplace_back({{0, isec}});
subsections.emplace_back({0, isec});
subsections.emplace_back(0, isec);

subsections is a vector of maps, and the emplacement incantation is beyond me. Clues welcome. Here are the relevant decls:

using SubsectionMap = std::map<uint32_t, InputSection *>;
class InputFile {
  std::vector<SubsectionMap> subsections;
};
gkm updated this revision to Diff 283999.Aug 7 2020, 12:49 PM

test case: move object file to end of arg list + add a .data section in order to prove that -sectcreate sections are place after sections from object files

int3 added inline comments.Aug 7 2020, 12:51 PM
lld/MachO/InputFiles.cpp
313

hm I'm not 100% familiar with what the C++ spec says about nested initialization lists... that not compiling is probably the reason why push_back was used elsewhere

This revision was automatically updated to reflect the committed changes.