Page MenuHomePhabricator

[lld-macho][easy] Fix segment max protection
ClosedPublic

Authored by int3 on Oct 14 2020, 12:48 PM.

Details

Reviewers
compnerd
Group Reviewers
Restricted Project
Commits
rG6cf244327b0d: [lld-macho][easy] Fix segment max protection
Summary

We should have maxprot == initprot for all non-i386 architectures, which
is what ld64 does.

Diff Detail

Event Timeline

int3 created this revision.Oct 14 2020, 12:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2020, 12:48 PM
int3 requested review of this revision.Oct 14 2020, 12:48 PM
compnerd added inline comments.
lld/MachO/OutputSegment.cpp
36

It seems that it would be better to replace initProt and maxProt with just prot? Or just filling in the differences that require this would be a good idea.

int3 added inline comments.Oct 15 2020, 9:11 AM
lld/MachO/OutputSegment.cpp
36

Does it make sense to fill in code that we're not going to be able to test?

int3 added inline comments.Oct 15 2020, 9:13 AM
lld/MachO/OutputSegment.cpp
36

I wrote it this way so it would be easy to "fill out" the missing bits when we support i386

compnerd added inline comments.Oct 21 2020, 8:53 AM
lld/MachO/OutputSegment.cpp
36

I think that it does make sense to fill it out now even if we cannot test it. Alternatively, we could defer the setup to the point where it is needed. In the current state, it is very confusing and not very valuable.

int3 added inline comments.Oct 28 2020, 2:23 PM
lld/MachO/OutputSegment.cpp
36

I think the assertion makes the intent clear -- they're two separate functions because on i386 their behaviors deviate. Moreover, making it an assertion ensures that when we do add support for i386, it'll be immediately obvious that a test needs to be written to cover this edge case.

This revision is now accepted and ready to land.Oct 30 2020, 9:13 AM
This revision was automatically updated to reflect the committed changes.