This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Emit load command LC_BUILD_VERSION
ClosedPublic

Authored by gkm on Aug 11 2020, 2:25 PM.

Diff Detail

Event Timeline

gkm created this revision.Aug 11 2020, 2:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2020, 2:25 PM
gkm requested review of this revision.Aug 11 2020, 2:25 PM
gkm added a comment.Aug 11 2020, 2:31 PM

@int3, For tests cases, I did the simple fix of adjusting addresses to accommodate the expansion of the load-command section. Should I expend the effort to generalize via patterns in order to quietly absorb future load-command expansions?

gkm planned changes to this revision.Aug 11 2020, 2:32 PM
int3 added a comment.EditedAug 11 2020, 2:43 PM

Should I expend the effort to generalize via patterns in order to quietly absorb future load-command expansions?

The reason those tests use hardcoded offsets instead of FileCheck's numeric substitutions is because of little-endianness. I'm not sure how they can be generalized while still checking for the things we care about. (Well, this is true for local-got.s at least. relocations.s just seems to be checking the little-endian encodings of the addresses, but doesn't check that it matches the big-endian form... this was an oversight)

lld/MachO/Writer.cpp
280

codebase convention is not to prefix member variables

lld/test/MachO/headerpad.s
14–15

nit: add spaces to align with column

gkm updated this revision to Diff 284914.Aug 11 2020, 3:19 PM

add testcase & respond to review feedback

gkm updated this revision to Diff 284932.Aug 11 2020, 4:33 PM

Remove spurious change to lld/MachO/CompactUnwind.cpp

int3 accepted this revision.Aug 11 2020, 5:26 PM

lgtm!

lld/MachO/Writer.cpp
254

nit: const ref

280

const ref

lld/test/MachO/lc-build-version.s
16
This revision is now accepted and ready to land.Aug 11 2020, 5:26 PM
gkm updated this revision to Diff 284947.Aug 11 2020, 6:37 PM

Add const qualifier to reference arg

gkm updated this revision to Diff 284949.Aug 11 2020, 6:47 PM

correct some omissions

gkm updated this revision to Diff 284950.Aug 11 2020, 6:52 PM

Tighten testcase by imposing sequencing on expected lines

Harbormaster completed remote builds in B68044: Diff 284949.
smeenai added inline comments.Aug 12 2020, 5:38 PM
lld/MachO/Writer.cpp
256

Nit: can this and platform be private?

This revision was landed with ongoing or failed builds.Aug 14 2020, 12:36 PM
This revision was automatically updated to reflect the committed changes.
mehdi_amini added inline comments.
lld/test/MachO/lc-build-version.s
13

This macro is a bit overly restrictive: our version of lld does not start with a leading 1 and fails this test.

I relaxed it in https://github.com/llvm/llvm-project/commit/22cbe40fa99795766f352c08a7c0748704c9d2d5 for now, if you would like a more specific checks I can propose this instead https://reviews.llvm.org/D86026 ?