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

[Nico Weber via Phabricator via llvm-commits]

thakis added a comment.

Comments addressed, thanks!

Re binding vs private externals: See also "references to internal symbol never need binding" comments in ld64.



================
Comment at: lld/MachO/Driver.cpp:528
+                           /*isExternal=*/true,
+                           /*isPrivateExtern=*/common->privateExtern);
   }
----------------
int3 wrote:
> nit: seems unnecessary to have the argument name in a comment when the argument itself is descriptive, but I guess it might look more uniform this way. up to you
Yup, done. That's a left-over from an earlier iteration.


================
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()) {}
----------------
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().


================
Comment at: lld/MachO/Symbols.h:114
   bool overridesWeakDef : 1;
+  bool privateExtern : 1;
 
----------------
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.


================
Comment at: lld/test/MachO/private-extern.s:117
+# RUN: llvm-nm -m %t/csclp | FileCheck --check-prefix=COMMONNONEXTERNAL %s
+# COMMONNONEXTERNAL: (__DATA,__common) non-external (was a private external) _foo
+
----------------
int3 wrote:
> how do you feel about hyphenating the check prefix?
No strong opinion. The advantage of no hyphens is that `-NOT`, `-DAG` etc look more obviously special, the downside is that it's a bit harder to read. Added hyphens.


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

https://reviews.llvm.org/D93609