[PATCH] D129062: [lld-macho] Handle user-provided dtrace symbols to avoid linking failure

[Vy Nguyen via Phabricator via llvm-commits]

oontvoo added inline comments.


================
Comment at: lld/MachO/Arch/ARM.cpp:44
+
+  void handleDtraceReloc(const Symbol *sym, const Reloc r,
+                         uint8_t *loc) const override;
----------------
Let's not copy the reloc.


================
Comment at: lld/MachO/Arch/ARM.cpp:177
+
+void ARM::handleDtraceReloc(const Symbol *sym, const Reloc r,
+                            uint8_t *loc) const {
----------------



================
Comment at: lld/MachO/Arch/ARM.cpp:179
+                            uint8_t *loc) const {
+  assert(r.type == ARM_RELOC_BR24 || r.type == ARM_THUMB_RELOC_BR22);
+
----------------
why do we need this assert if the `default` in the following `switch` already handles the checking?


================
Comment at: lld/MachO/Arch/ARM.cpp:187
+      // change call site to a NOP
+      write32le(loc, 0xE1A00000);
+    } else if (sym->getName().startswith("___dtrace_isenabled")) {
----------------
these magic numbers are a bit unfortunate ... i wonder if it's better to use constants?


================
Comment at: lld/MachO/Arch/ARM.cpp:209
+    } else {
+      error("Unrecognized dtrace symbol prefix: " + sym->getName());
+    }
----------------
consider using `toString(Symbol*)` instead. (That way, we get the demangled vs non-demangled handling for the symbol name, depending on whether `-demangle` is set)


================
Comment at: lld/MachO/Arch/ARM64Common.h:33
+
+  void handleDtraceReloc(const Symbol *sym, const Reloc r,
+                         uint8_t *loc) const override;
----------------



================
Comment at: lld/MachO/Arch/X86_64.cpp:42
+
+  void handleDtraceReloc(const Symbol *sym, const Reloc r,
+                         uint8_t *loc) const override;
----------------



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

https://reviews.llvm.org/D129062