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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/MachO/Driver.cpp:40
+// Create prefix string literals used in Options.td
+#define PREFIX(NAME, VALUE) const char *const NAME[] = VALUE;
+#include "Options.inc"
----------------
`const char *NAME[] = VALUE;`


================
Comment at: lld/MachO/Driver.cpp:74
+  if (s != "x86_64")
+    error("missing or unsupported -arch " + s);
+  return createX86_64TargetInfo();
----------------
Add a `-arch i386` test. There is currently no test.


================
Comment at: lld/MachO/Driver.cpp:89
+  default:
+    error(path + ": unhandled file type");
+  }
----------------
Add a test similar to `test/ELF/invalid/executable.s`


================
Comment at: lld/MachO/Driver.cpp:126
+  if (!isa<Defined>(config->entry)) {
+    error("undefined symbol: " + config->entry->getName());
+    return false;
----------------
Add an `-e ` test with an undefined symbol.


================
Comment at: lld/MachO/Driver.cpp:139
+        getOrCreateOutputSegment(isec->segname, VM_PROT_READ | VM_PROT_WRITE);
+    std::vector<InputSection *> &vec = os->sections[isec->name];
+    vec.push_back(isec);
----------------
`vec` can be omitted.


================
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
----------------
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?


================
Comment at: lld/MachO/InputFiles.cpp:70
+  if (auto ec = mbOrErr.getError()) {
+    error("cannot open " + path + ": " + ec.message());
+    return None;
----------------
Add a `cannot open ` test. See `test/ELF/basic.s` (I don't think placing many things in one file is good but you can copy one of the tests (%t.no.such.file))


================
Comment at: lld/MachO/InputFiles.cpp:84
+
+  for (size_t i = 0; i < hdr->ncmds; ++i) {
+    auto *cmd = (const load_command *)p;
----------------
`for (uint32_t i =0, n = hdr->ncmds, i != n; ++i)`

https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop


================
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)
----------------
reinterpret_cast is more idiomatic.


================
Comment at: lld/MachO/InputFiles.cpp:106
+    isec->data = {buf + sec.offset, sec.size};
+    isec->align = 1 << sec.align;
+    isec->flags = sec.flags;
----------------
What if sec.align is >= 32 ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75382