Page MenuHomePhabricator

Extract LC_CODE_SIGNATURE related implementation out of LLD
ClosedPublic

Authored by nuriamari on Sep 14 2021, 6:42 PM.

Details

Summary

Move the functionality in lld that handles writing of the LC_CODE_SIGNATURE load command and associated data section to a central reusable location.

This change is in preparation for another change that modifies llvm-objcopy to reproduce the LC_CODE_SIGNATURE load command and corresponding
data section to maintain the validity of signed macho object files passed through llvm-objcopy.

Diff Detail

Event Timeline

nuriamari created this revision.Sep 14 2021, 6:42 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
nuriamari edited the summary of this revision. (Show Details)Sep 15 2021, 6:50 AM
nuriamari added reviewers: int3, drodriguez.
nuriamari published this revision for review.Sep 15 2021, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2021, 9:06 AM

Move itself looks fine, but why is there a new lld test in here? This should be behavior-preserving, so I wouldn't expect test updates.

If you want to add a new test to lld independently from this change (?), I'd put that in a separate patch.

Move itself looks fine, but why is there a new lld test in here? This should be behavior-preserving, so I wouldn't expect test updates.

If you want to add a new test to lld independently from this change (?), I'd put that in a separate patch.

The new test is to ensure no behavior is broken. The existing code signature related test does not test that the hash was performed properly, just that the load command is included. This test is to make sure the move doesn't break the signature generation.

Move itself looks fine, but why is there a new lld test in here? This should be behavior-preserving, so I wouldn't expect test updates.

If you want to add a new test to lld independently from this change (?), I'd put that in a separate patch.

The new test is to ensure no behavior is broken. The existing code signature related test does not test that the hash was performed properly, just that the load command is included. This test is to make sure the move doesn't break the signature generation.

Can we land that in its own patch? :)

Remove new test, now included in separate patch

Move itself looks fine, but why is there a new lld test in here? This should be behavior-preserving, so I wouldn't expect test updates.

If you want to add a new test to lld independently from this change (?), I'd put that in a separate patch.

The new test is to ensure no behavior is broken. The existing code signature related test does not test that the hash was performed properly, just that the load command is included. This test is to make sure the move doesn't break the signature generation.

Can we land that in its own patch? :)

Sure :) https://reviews.llvm.org/D109840

thevinster added inline comments.
lld/MachO/SyntheticSections.cpp
1148–1149

Any specific reason for not namespacing object? :)

1160–1167

It seems redundant to have both writeTo and writeHashes now that it's combined. Comment makes since in the context of this diff but it doesn't make too much sense outside of the context unless people are referring to this diff. Personally, I would rename all places that were using writeHashes to writeTo and get rid of writeHashes completely from this class (unless there other issues preventing this)

oontvoo added inline comments.
lld/MachO/SyntheticSections.cpp
1148–1149

this is now a stray comment?

llvm/include/llvm/Object/MachO.h
737–750

Please consider re-arranging the members. I think the convention is to leave private fields at the end of the class, after private methods. (Also explicit modifier is preferred).

More generally, the order of members are <PUBLIC>, <PROTECTED>, <PRIVATE>, then within each, methods go before fields.

llvm/lib/Object/CodeSignatureSection.cpp
28

not needed - unless you're going to add some text here?

nuriamari updated this revision to Diff 372824.Sep 15 2021, 3:55 PM
nuriamari marked 4 inline comments as done.

Fix stray comment, rearrange type definition and clean up static asserts

lld/MachO/SyntheticSections.cpp
1148–1149

If I namespace object, there is a collision between the CodeSignatureSection defined in SyntheticSections.h and the desired definition in MachO.h.

1160–1167

The base class requires writeTo to be defined and writeTo can't cleanly be used to write the hashes since they can only be written once everything else in the binary is written.

oontvoo accepted this revision as: oontvoo.Sep 15 2021, 7:54 PM

LGTM

llvm/lib/Object/CodeSignatureSection.cpp
28

Oops, sorry about this suggestion - I forgot LLVM is still on C++14.

This revision is now accepted and ready to land.Sep 15 2021, 7:54 PM
nuriamari updated this revision to Diff 372858.Sep 15 2021, 8:04 PM
nuriamari marked an inline comment as done.

Satisfy clang-tidy / clang-format

nuriamari updated this revision to Diff 372859.Sep 15 2021, 8:08 PM

Restore static asserts

int3 added inline comments.Sep 16 2021, 8:10 AM
lld/MachO/SyntheticSections.cpp
1155–1157

can we construct this once in CodeSignatureSection::finalize()? (so CodeSignatureSection would have a std::unique_ptr<object::CodeSignatureSection> member that can be reused in writeHashes)

nuriamari updated this revision to Diff 372958.Sep 16 2021, 8:29 AM

Construct object::CodeSignature object once

nuriamari marked an inline comment as done.Sep 16 2021, 8:43 AM
int3 accepted this revision.Sep 16 2021, 8:48 AM

lgtm

This revision was automatically updated to reflect the committed changes.