[PATCH] D78270: [lld-macho] Support calls to functions in dylibs

[Shoaib Meenai via Phabricator via llvm-commits]

smeenai added inline comments.


================
Comment at: lld/MachO/SyntheticSections.cpp:265
+    error("TODO: Support larger dylib symbol ordinals");
+    return opstreamOffset;
+  }
----------------
int3 wrote:
> smeenai wrote:
> > What's the purpose of this return, given we're gonna error anyway?
> `error` doesn't cause the program to exit, for that we need `fatal`. But I guess this should really be a `fatal`.
I know that `error` doesn't cause the program to exit right away, but it will eventually cause the program to exit with an error. Given that, I was wondering if not having the `return` here and instead falling through to the `os <<` below would cause any harm.


================
Comment at: lld/test/MachO/dylink-lazy.s:48
+# CHECK-NEXT:    jmp 0x[[#STUB_HELPER_ENTRY]]
+# CHECK-NEXT:    pushq $
+# CHECK-NEXT:    jmp 0x[[#STUB_HELPER_ENTRY]]
----------------
int3 wrote:
> smeenai wrote:
> > How come we aren't checking the value here?
> I guess you mean on line 48? That's the offset into the bind opcode stream, and there's no nice arithmetic to describe it. Hard-coding the value will be brittle -- it will vary based on which entries appear earlier in the stream, and how large they are (which includes the length of their symbol names). There's the counterargument that these things aren't likely to change that often in our tests of course...
Yeah, we should strike a balance between testing everything and having brittle tests, but in this scenario, assuming we don't modify the test itself, I'd say it's worth checking the actual offset. I'd even consider checking the bind opcode bytes we're emitting (assuming those aren't likely to change from future linker changes, only from future changes to this test itself), to ensure we don't accidentally regress those in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78270