Page MenuHomePhabricator

[lld-macho] Set __PAGEZERO size to 4GB
ClosedPublic

Authored by int3 on Mon, May 18, 9:09 PM.

Details

Summary

That's what ld64 uses for 64-bit targets. I figured it's best
to make this change sooner rather than later since a bunch of our tests
are relying on hardcoded addresses that depend on this value.

Depends on D80169.

Diff Detail

Event Timeline

int3 created this revision.Mon, May 18, 9:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, May 18, 9:09 PM
smeenai requested changes to this revision.Tue, May 19, 1:43 PM

Thanks for fixing this! It'll make it way easier to compare results with ld64.

Requesting changes cos I think we're gonna mess up entryoff as this stands, and we definitely need to add test coverage for that.

lld/MachO/Target.h
30

It's really confusing to have PageZeroSize and ImageBase be different, and for that matter, to have both of them.

With this diff, we're only using ImageBase in one place: https://github.com/llvm/llvm-project/blob/master/lld/MachO/Writer.cpp#L171. From looking into LC_MAIN, it seems like the entryoff is supposed to be an offset in the file rather than a virtual address, so computing it in terms of getVA is confusing. We should also fix that and then get rid of either ImageBase or PageZeroSize.

For that matter, is our LC_MAIN entryoff calculation gonna be correct after this? I have a hard time imagining that, and we don't have any tests for it :/

This revision now requires changes to proceed.Tue, May 19, 1:43 PM
int3 marked 2 inline comments as done.Tue, May 19, 3:10 PM
int3 added inline comments.
lld/MachO/Target.h
30

oops, good catch...

int3 updated this revision to Diff 265057.Tue, May 19, 3:10 PM
int3 marked an inline comment as done.

fix and add test

int3 updated this revision to Diff 265058.Tue, May 19, 3:13 PM

make ENTRYOFF a numeric var

smeenai accepted this revision.Tue, May 19, 3:34 PM

Looks great, thanks!

lld/MachO/Symbols.h
111

This is *definitely* follow-up territory, but we should figure out if it ever makes sense for getFileOffset to be called on a non-defined symbol, and if not, just fail loudly in that case instead of silently returning 0.

This revision is now accepted and ready to land.Tue, May 19, 3:34 PM
int3 marked an inline comment as done.Tue, May 19, 4:12 PM
int3 added inline comments.
lld/MachO/Symbols.h
111

oh good point. I doubt it would ever be valid; I can just insert a fatal here. Does it really need to be in a follow-up?

smeenai added inline comments.Tue, May 19, 4:36 PM
lld/MachO/Symbols.h
111

It doesn't need to be a follow-up. I just didn't mean the comment to block this diff, but folding it in here is great too.

smeenai added inline comments.Wed, May 20, 12:32 PM
lld/MachO/Symbols.h
111

I'd probably use llvm_unreachable, since reaching this would be purely a logic error in the linker rather then e.g. something that could be triggered by broken or unsupported input.

int3 updated this revision to Diff 265372.Wed, May 20, 3:53 PM

llvm_unreachable

This revision was automatically updated to reflect the committed changes.
int3 added a comment.Tue, Jun 2, 3:28 PM

Thanks for the heads up, just pushed a likely fix and will monitor http://lab.llvm.org:8011/builders/lld-x86_64-win. I'll set up a local Windows VM to catch future such issues earlier...