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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Thanks for working on this!

> I'm not sure whether support for dtrace is expected in lld, so for now I didn't add codes to make lld emit __dof section like ld64, and only made it possible to link with dtrace symbols provided. If this feature is needed, I can add that part in Dtrace.cpp & Dtrace.h.

I'm not aware of anyone asking for this right now, but it does seem like something we'll want eventually :) feel free to work on that in a followup if you're interested



================
Comment at: lld/MachO/Arch/ARM.cpp:177-178
+
+void ARM::handleDtraceReloc(const Symbol *sym, const Reloc r,
+                            uint8_t *loc) const {
+  assert(r.type == ARM_RELOC_BR24 || r.type == ARM_THUMB_RELOC_BR22);
----------------
let's include a comment about how this only implements `-no_dtrace_dof`, and that this deviates from ld64's default behavior


================
Comment at: lld/MachO/Arch/ARM.cpp:184
+    if (sym->getName().startswith("___dtrace_probe")) {
+      // ld64: fixup is kindStoreARMDtraceCallSiteNop
+      if (config->outputType == MH_OBJECT) {
----------------
I don't think we should make direct references to ld64 code like this


================
Comment at: lld/MachO/Arch/ARM.cpp:204-206
+      if (config->outputType == MH_OBJECT) {
+        return;
+      }
----------------
code convention is to omit braces for one-liners


================
Comment at: lld/MachO/Arch/ARM.cpp:224
+}
\ No newline at end of file

----------------
nit: can you set your editor to include newlines at EOF?


================
Comment at: lld/MachO/Arch/ARM64Common.cpp:117-121
+  if (!r.pcrel)
+    error("Not pcrel and ARM64_RELOC_BRANCH26 not supported");
+
+  if (r.length != 2)
+    error("r_length != 2 and ARM64_RELOC_BRANCH26 not supported");
----------------
we should handle this generically in `validateRelocationInfo` (if we aren't doing that already) rather than here


================
Comment at: lld/MachO/Dtrace.cpp:1-2
+//===- Dtrace.cpp
+//----------------------------------------------------------===//
+//
----------------
nit: fix formatting


================
Comment at: lld/MachO/Dtrace.cpp:17
+
+bool lld::macho::isDtraceSym(StringRef name) {
+  if (name.substr(0, 10) == "___dtrace_") {
----------------



================
Comment at: lld/MachO/Dtrace.cpp:18
+bool lld::macho::isDtraceSym(StringRef name) {
+  if (name.substr(0, 10) == "___dtrace_") {
+    return true;
----------------
could use `startswith()`


================
Comment at: lld/MachO/Dtrace.cpp:24-40
+void lld::macho::resolveDtraceSymbol(const Symbol *sym, const Reloc r,
+                                     uint8_t *loc) {
+  // Dtrace symbol should be undefined
+  assert(sym && isa<Undefined>(sym));
+
+  switch (target->cpuType) {
+  case CPU_TYPE_X86_64:
----------------
not sure this function is necessary. Couldn't we hoist the `error()` into `TargetInfo::handleDtraceReloc` and call that directly?


================
Comment at: lld/test/MachO/arm-dtrace.ll:35-61
+  %1 = alloca i32, align 4
+  %2 = alloca i32, align 4
+  %3 = alloca i32, align 4
+  store i32 0, i32* %1, align 4
+  %4 = call i32 @"__dtrace_isenabled$Foo$added$v1"()
+  store i32 %4, i32* %2, align 4
+  call void asm sideeffect "", ""() #2, !srcloc !6
----------------
can we minimize the test case? probably most of the instructions aside from the `call`s to dtrace functions can be removed. The `!srcloc` references too


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129062