[PATCH] D93609: [lld/mac] Implement support for private extern symbols

Nico Weber via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Dec 21 18:23:00 PST 2020


thakis marked 4 inline comments as done.
thakis added a comment.

Thanks! Submitting.



================
Comment at: lld/MachO/Symbols.h:51
+  StringRef getName() const {
+    if (nameSize == (uint32_t)-1)
+      nameSize = strlen(nameData);
----------------
int3 wrote:
> is the cast necessary?
../../lld/MachO/Symbols.h:51:18: warning: comparison of integers of different signs: 'uint32_t' (aka 'unsigned int') and 'int' [-Wsign-compare]


================
Comment at: lld/test/MachO/weak-private-extern.s:20-21
+# RUN: obj2yaml %t.dylib | FileCheck --check-prefix=YAML %s
+# YAML-NOT: n_desc:          128
+# YAML-NOT: n_desc:          128
+
----------------
int3 wrote:
> other reviewers have expressed a preference against using `obj2yaml` in tests unless absolutely necessary... though I don't remember the exact reasons (cc @smeenai). In this case I think we could use `llvm-readobj --syms` -- it would make the expected value more readable too
Oh yeah, that's nicer, thanks.


================
Comment at: lld/test/MachO/weak-private-extern.s:25
+_use:
+  mov _weak_private_extern_gotpcrel at GOTPCREL(%rip), %rax
+  callq _weak_private_extern
----------------
int3 wrote:
> I was going to suggest that it might be worth testing TLV weak private externs too, then I realized that we lacked a test for regular TLV weakdefs 🤭
> 
> we could address both in a separate diff I guess
Will send you tests for TLV weaks tomorrow :)


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

https://reviews.llvm.org/D93609



More information about the llvm-commits mailing list