[PATCH] D75382: [lld] Initial commit for new Mach-O backend

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Arch/X86_64.cpp:51
+    error("TODO: Unhandled relocation type");
+  }
+}
----------------
good point, hadn't noticed that `Writer` would exit if `errorCount` was nonzero.


================
Comment at: lld/MachO/InputFiles.cpp:24
+// A section can have multiple symbols. A symbol that does not have
+// N_ALT_ENTRY attribute indicates a beginning of a subsection. Therefore, by
+// default, a symbol is always present at beginning of each subsection. A
----------------
MaskRay wrote:
> the beginning
> 
> Do you mean that a symbol defined in the section must be at the beginning of a subsection, unless the N_ALT_ENTRY attribute is set?
It was @ruiu who originally wrote this, but I believe it means that a symbol w/o the N_ALT_ENTRY attribute defines the beginning of a subsection. So your statement should be true by definition. Maybe "by default" should be replaced with "by definition"...


================
Comment at: lld/MachO/InputFiles.cpp:85
+  for (size_t i = 0; i < hdr->ncmds; ++i) {
+    auto *cmd = (const load_command *)p;
+    if (cmd->cmd == type)
----------------
MaskRay wrote:
> reinterpret_cast is more idiomatic.
fair enough. I've converted all the C-style casts I could find.


================
Comment at: lld/MachO/InputFiles.cpp:106
+    isec->data = {buf + sec.offset, sec.size};
+    isec->align = 1 << sec.align;
+    isec->flags = sec.flags;
----------------
MaskRay wrote:
> What if sec.align is >= 32 ?
I haven't found the logic in ld64 that handles this, but some experimentation (creating object files with high alignments using yaml2obj and then passing it to ld64) suggests it does applies `mod 256` to `sec.align` w/o emitting warnings or anything. Obviously 2 ** 256 is still ridiculously large, and ultimately `ld64` emits a 2GB object file, so it looks like some silent overflows are happening.

I'm leaning towards applying `mod 32` here and emitting a warning.


================
Comment at: lld/MachO/Options.td:11
+
+def version: Flag<["--"], "version">, HelpText<"Display the version number and exit">;
+
----------------
MaskRay wrote:
> --version uses a double-dash form while other long options use single-dash form. Can you confirm?
hm, ld64 actually doesn't have a `version` flag at all, I was just copying lld-elf here... but I suppose it would make sense for this to be single-dash to be consistent with the other flags here (and in ld64)


================
Comment at: lld/MachO/Target.h:18
+enum {
+  PageSize = 4096,
+  ImageBase = 4096,
----------------
MaskRay wrote:
> `pageSize`
> `imageBase`
clang-tidy specifies enums to be CamelCase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75382