[PATCH] D77006: [lld-macho] Support reading of universal binaries

Tue Apr 28 23:57:37 PDT 2020

int3 marked an inline comment as done.
int3 added inline comments.

================
Comment at: lld/test/MachO/fat-arch.s:10
+# RUN:    FileCheck %s -DFILE=%t.noarch.o
+# CHECK: Unable to find matching architecture in [[FILE]]
+# CHECK: undefined symbol: _main
----------------
MaskRay wrote:
> int3 wrote:
> > MaskRay wrote:
> > > int3 wrote:
> > > > smeenai wrote:
> > > > > MaskRay wrote:
> > > > > > 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.
> > > > > 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.
> > > > > 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.
> > > No. Color code sequences are not emitted when stderr is connected to a regular file/pipe instead of a pty.
> > 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
> Most `test/ELF/*` error/warning tests include `error: ` or `warning: `.
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!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77006/new/

https://reviews.llvm.org/D77006