[PATCH] D101080: [lld/mac] Implement support for .weak_def_can_be_hidden

[Jez Ng via Phabricator via llvm-commits]

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

love the comments!



================
Comment at: lld/MachO/InputFiles.cpp:450
+    // give it visibility as if the function was built without
+    // fvisibility-inlines-hidden.
+    // If both functions have the same contents, this will have the same
----------------



================
Comment at: lld/MachO/InputFiles.cpp:457-459
+    // that's not isPrivateExtern but isWeakDefCanBeHidden technically
+    // should produce one
+    // that's not isPrivateExtern and isWeakDefCanBeHidden. That matters
----------------
nit: I assume you aligned the paragraph this way so that the various references to isWeakDefCanBeHidden would line up vertically for easy comparison. To make it clearer, how about changing "and isWeakDefCanBeHidden" to "but isWeakDefCanBeHidden" so it's clearly identical to line 457?


================
Comment at: lld/MachO/InputFiles.cpp:460
+    // that's not isPrivateExtern and isWeakDefCanBeHidden. That matters
+    // with ld64's semantics. With lld's semantics there's no observable
+    // difference between a symbol that's weak and isWeakDefCanBeHidden
----------------
Did I understand it correctly?


================
Comment at: lld/MachO/InputFiles.cpp:461
+    // with ld64's semantics. With lld's semantics there's no observable
+    // difference between a symbol that's weak and isWeakDefCanBeHidden
+    // or one that's privateExtern -- neither makes it into the dynamic
----------------
I think the "weak and" is redundant?


================
Comment at: lld/test/MachO/weak-def-can-be-hidden.s:19-24
+# Basics: A weak_def_can_be_hidden symbol should not be in the output's
+# export table, nor in the weak bind table, and references to it from
+# within the dylib should not use weak indirect lookups.
+# Think: Inline function compiled without any -fvisibility flags, inline
+# function does not have its address taken. In -O2 compiles, GlobalOpt will
+# upgrade the inline function from .weak_definition to .weak_def_can_be_hidden.
----------------
(same for other non-filecheck comments)


I'm wondering if it would be clearer to have this context in InputFiles.cpp 🤔but either seems fine


================
Comment at: lld/test/MachO/weak-def-can-be-hidden.s:56
+# looks like a bug in ld64 (?) If you change lit.local.cfg to set %lld to ld to
+# test compatibility, you have to add some arbitary suffix to these two lines:
+# HEADERS-NOT: WEAK_DEFINES
----------------
s/arbitary/arbitrary/

but also: maybe "you have to remove the HEADERS check" would be clearer? I had to think for a second to make the link between adding a suffix and ensuring that a `-NOT` check wouldn't match


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

https://reviews.llvm.org/D101080