[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:31
+
+using namespace lld;
+using namespace lld::macho;
----------------
Reorder `llvm` namespaces before `lld`.


================
Comment at: lld/MachO/Driver.cpp:66
+
+  for (auto *arg : args.filtered(OPT_UNKNOWN))
+    error("unknown argument: " + arg->getSpelling());
----------------
auto -> `opt::Arg`


================
Comment at: lld/MachO/Driver.cpp:147
+  if (canExitEarly) {
+    exitLld(errorCount() ? 1 : 0);
+  }
----------------
excess parentheses


================
Comment at: lld/MachO/InputFiles.cpp:103
+    isec->file = this;
+    isec->name = StringRef(sec.sectname, strnlen(sec.sectname, 16));
+    isec->segname = StringRef(sec.segname, strnlen(sec.segname, 16));
----------------
`MachO/section-names.s` should test a long section name.


================
Comment at: lld/MachO/InputFiles.cpp:104
+    isec->name = StringRef(sec.sectname, strnlen(sec.sectname, 16));
+    isec->segname = StringRef(sec.segname, strnlen(sec.segname, 16));
+    isec->data = {buf + sec.offset, sec.size};
----------------
There should be a test.


================
Comment at: lld/MachO/InputFiles.cpp:124
+    if (anyRel.r_word0 & R_SCATTERED) {
+      llvm_unreachable("TODO: Scattered relocations not supported");
+    } else {
----------------
Logically unreachable code uses llvm_unreachable. Unimplemented features should call `error()` instead.


================
Comment at: lld/MachO/InputFiles.cpp:148
+        ArrayRef<const section_64>{(const section_64 *)(c + 1), c->nsects};
+    sections = parseSections(objSections);
+  }
----------------
```
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_vector.h:398:7: error: static_assert failed due to requirement 'is_same<llvm::MachO
::section_64, const llvm::MachO::section_64>::value' "std::vector must have a non-const, non-volatile value_type"
      static_assert(is_same<typename remove_cv<_Tp>::type, _Tp>::value,
```


================
Comment at: lld/MachO/Target.cpp:20
+
+struct X86_64 : TargetInfo {
+  X86_64();
----------------
How many targets do Mach-O have? If it can be larger, like `ELF/Arch/X86_64.cpp`, we can create `MachO/Arch` accordingly.


================
Comment at: lld/MachO/Target.cpp:36
+  default:
+    llvm_unreachable("TODO: Unhandled relocation type");
+  }
----------------
Logically unreachable code uses llvm_unreachable. Unimplemented relocation types should call error() instead.


================
Comment at: lld/MachO/Writer.cpp:249
+private:
+  StringRef path = "/usr/lib/dyld";
+};
----------------
Is it a const?

Who defines it as /usr/lib/dyld ?


================
Comment at: lld/test/MachO/duplicate-symbol.s:4
+# RUN: llvm-mc -filetype=obj -triple=x86_64-apple-darwin %s -o %t-dup.o
+# RUN: not lld -flavor darwinnew -o %t %t-dup.o %t.o 2>&1 | FileCheck %s
+
----------------
Does `lld -flavor darwinnew -o /dev/null %t.o %t.o` work?


================
Comment at: lld/test/MachO/relocations.s:11
+_main:
+  pushq %rbp
+  movq %rsp, %rbp
----------------
Delete unneeded instructions.


================
Comment at: lld/test/MachO/section-names.s:23
+        mov $0, %rax
+        ret
+
----------------
too much indentation


================
Comment at: lld/test/MachO/undefined-entry.s:10
+_not_main:
+        mov $0, %rax
+        ret
----------------
8-space indentation may be too much. 2 is fine.
If the instruction is not significant, delete `mov $0, %rax`


================
Comment at: lld/tools/lld/CMakeLists.txt:15
   lldELF
+  lldMachO2
   lldMinGW
----------------
I hope we can name it lldMacho.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75382