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

[Jez Ng via Phabricator via llvm-commits]

int3 accepted this revision.
int3 added a subscriber: smeenai.
int3 added a comment.
This revision is now accepted and ready to land.

nice work!



================
Comment at: lld/MachO/SymbolTable.cpp:15
 #include "lld/Common/Memory.h"
+#include <assert.h>
 
----------------
leftover include?


================
Comment at: lld/MachO/Symbols.h:51
+  StringRef getName() const {
+    if (nameSize == (uint32_t)-1)
+      nameSize = strlen(nameData);
----------------
is the cast necessary?


================
Comment at: lld/MachO/Symbols.h:28
 struct StringRefZ {
-  StringRefZ(const char *s) : data(s), size(-1) {}
+  StringRefZ(const char *s) : StringRefZ(StringRef(s)) {}
   StringRefZ(StringRef s) : data(s.data()), size(s.size()) {}
----------------
thakis wrote:
> int3 wrote:
> > This defeats the purpose of using `StringRefZ` over `StringRef` though -- it's meant to be a performance optimization (see the comment on the identically-named class in lld/ELF... I probably should have copied it over and/or factored the class out)
> > 
> > Would a `constexpr` constructor work here?
> This ctor is exclusively used for the `__dyld_private` symbol, so performance probably isn't all that important :) Do you expect this to be used more often? Maybe it's not intentional that it's used so little?
> 
> But sure, undid this and instead imported the technique for dealing with this from ELF to Symbol::getName().
oops, not having that ctor used elsewhere was an oversight I guess


================
Comment at: lld/MachO/Symbols.h:114
   bool overridesWeakDef : 1;
+  bool privateExtern : 1;
 
----------------
thakis wrote:
> int3 wrote:
> > how about combining `privateExtern` and `external` into one enum, so that the invalid state of `external = false, privateExtern = true` is not representable?
> The current setup has the advantage that the `external` bit can stay const, which is why I when with two independent bits. It's also closer to how things look in the input and output files. ld64 does this dance where it converts all file bits to its own concepts at input and back at output and I find that more confusing then helpful.
> 
> I don't feel terribly strongly about that though. If you want I can change it to an enum.
ah fair enough. it's fine as-is


================
Comment at: lld/test/MachO/private-extern.s:105
+
+## for common symbols the larger one wins.
+## - smaller private extern + larger extern = larger extern
----------------
ultra nit


================
Comment at: lld/test/MachO/weak-private-extern.s:15
+## Check that N_WEAK_DEF isn't set in the symbol table.
+## This is different from ld64, which makes privat extern weak symbols non-weak
+## for binds and relocations, but it still marks them as weak in the symbol table.
----------------



================
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
+
----------------
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


================
Comment at: lld/test/MachO/weak-private-extern.s:25
+_use:
+  mov _weak_private_extern_gotpcrel at GOTPCREL(%rip), %rax
+  callq _weak_private_extern
----------------
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


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

https://reviews.llvm.org/D93609