This is an archive of the discontinued LLVM Phabricator instance.

[lld-macho] Support reading of universal binaries
ClosedPublic

Authored by int3 on Mar 28 2020, 11:03 PM.

Diff Detail

Event Timeline

int3 created this revision.Mar 28 2020, 11:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2020, 11:03 PM
int3 edited the summary of this revision. (Show Details)Mar 28 2020, 11:18 PM
int3 edited the summary of this revision. (Show Details)
int3 added subscribers: ruiu, pcc.
int3 updated this revision to Diff 253441.Mar 29 2020, 11:26 AM

output to /dev/null

ruiu accepted this revision.Mar 31 2020, 1:53 AM

LGTM

This revision is now accepted and ready to land.Mar 31 2020, 1:53 AM
MaskRay accepted this revision.Mar 31 2020, 10:25 AM

LGTM, with nits.

lld/test/MachO/fat-arch.s
11

Add more context the the diagnostics, probably error:

It is also a convention to not capitalize the diagnostics, see clang, ld.lld and binutils/LLVM binary utilities' output.

smeenai added inline comments.Mar 31 2020, 2:49 PM
lld/MachO/InputFiles.cpp
90

We may wanna add some bounds checking here to handle malformed fat files, e.g. if nfat_arch is an absurdly large value and we go over bounds. Idk if yaml2obj is capable of generating a malformed fat file of that sort, but we should at least have some checks.

smeenai added inline comments.Mar 31 2020, 2:50 PM
lld/test/MachO/fat-arch.s
11

I believe ld64 treats this scenario as a warning, not an error. I wouldn't mind us treating this as an error (at least until we get more experience with the new linker and whether that's reasonable behavior), because in my experience, a mistake like this usually leads to undefined symbols in your link anyway, and it's less confusing if you get an error about the issue right away.

int3 updated this revision to Diff 254081.Mar 31 2020, 8:40 PM
int3 marked 2 inline comments as done.

address comments

lld/test/MachO/fat-arch.s
11

Add more context the the diagnostics, probably error:

lld/Common/ErrorHandler.h always prints out ANSI color markers around its warning: and error: prefixes. We could use regexes to ignore the colors I guess but it's kind of ugly...

I'll make it an error.

int3 updated this revision to Diff 257878.Apr 15 2020, 3:45 PM

copy the file path when returning a MemoryBufferRef

int3 updated this revision to Diff 260840.Apr 28 2020, 10:40 PM

fix ASAN issue, add dep on llvm-lipo

int3 added a comment.Apr 28 2020, 10:40 PM

I realize I'd neglected to add llvm-lipo as a test dependency. Just fixed that, but I've added it as a dependency for all lld test suites (including ELF and COFF); not sure if it's possible to add it for just check-lld-macho.

MaskRay added inline comments.Apr 28 2020, 10:49 PM
lld/test/MachO/invalid-fat-narch.s
8

You may omit --- and ...

MaskRay added inline comments.Apr 28 2020, 10:51 PM
lld/test/MachO/fat-arch.s
11

No. Color code sequences are not emitted when stderr is connected to a regular file/pipe instead of a pty.

int3 marked 3 inline comments as done.Apr 28 2020, 11:05 PM
int3 added inline comments.
lld/test/MachO/fat-arch.s
11

I agree that's the way it *should* work but I don't think that has been implemented... just from a glance at ErrorHandler.cpp anyway. It is definitely still producing the color markers when I run it under llvm-lit locally though

lld/test/MachO/invalid-fat-narch.s
8

ah thanks, wasn't too familiar with YAML syntax

int3 updated this revision to Diff 260842.Apr 28 2020, 11:05 PM
int3 marked an inline comment as done.

remove redundant yaml stuff

MaskRay added inline comments.Apr 28 2020, 11:15 PM
lld/test/MachO/fat-arch.s
11

Most test/ELF/* error/warning tests include error: or warning: .

int3 marked an inline comment as done.Apr 28 2020, 11:39 PM
int3 added inline comments.
lld/test/MachO/fat-arch.s
11

Aha, we weren't setting up the error logger properly in lld-macho's Driver.cpp. I'll put up a diff to fix that. Thanks!

int3 updated this revision to Diff 260849.Apr 29 2020, 12:01 AM

add error: prefix

This revision was automatically updated to reflect the committed changes.