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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Just some nits but otherwise lgtm :)



================
Comment at: lld/MachO/Arch/ARM.cpp:182-183
+    if (sym->getName().startswith("___dtrace_probe")) {
+      if (config->outputType == MH_OBJECT)
+        return;
+      // change call site to a NOP
----------------
could we hoist this to the top of the function? seems like we'd just never want to modify any dtrace refs if we are outputting an object file


================
Comment at: lld/MachO/Arch/X86_64.cpp:210-213
+    loc[0] = 0x0F;  // 4-byte nop
+    loc[1] = 0x1F;
+    loc[2] = 0x40;
+    loc[3] = 0x00;
----------------
I kinda preferred the previous version where you used `write32le` here. I think @BertalanD's suggestion of writing to `loc[-1]` was to make it clear that we are only modifying a single byte. But if we're emitting a 4-byte instruction, `write32le()` makes sense

i.e. I'm suggesting

```
loc[-1] = 0x90;
write32le(loc, 0x00401F0F);
```


================
Comment at: lld/test/MachO/arm-dtrace.s:5
+# RUN: llvm-mc -filetype=obj -triple=armv4t-apple-macosx12.0.0 %t/armv4t-dtrace.s -o %t/armv4t-dtrace.o
+# RUN: %lld -demangle -dynamic -arch armv4t -platform_version macos 12.0.0 12.3 -o %t/armv4t-dtrace %t/armv4t-dtrace.o
+
----------------
are the `-demangle -dynamic` flags actually necessary? Also, `%lld` by default specifies a platform version of 11.0. Any reason we can't use that default?


================
Comment at: lld/test/MachO/arm-dtrace.s:30-31
+	bl	___dtrace_isenabled$Foo$added$v1
+	beq	LBB0_4
+	b	LBB0_1
+LBB0_1:
----------------
are these branching instructions necessary? doesn't seem like they really test anything


================
Comment at: lld/test/MachO/x86_64-dtrace.s:10-16
+# CHECK: 33 c0                        xorl    %eax, %eax
+# CHECK-NEXT: 90                           nop
+# CHECK-NEXT: 90                           nop
+# CHECK-NEXT: 90                           nop
+
+# CHECK: 90                           nop
+# CHECK-NEXT: 0f 1f 40 00                  nopl    (%rax)
----------------
nit: can we column-align the right hand side?


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

https://reviews.llvm.org/D129062