[PATCH] D126046: [lld-macho] Support -non_global_symbols_strip_list, -non_global_symbols_no_strip_list, -x

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

Sorry for the belated review. Took me a little longer than expected to get back in the swing of things :)



================
Comment at: lld/MachO/Driver.cpp:941
 
+static void handleSymbolPatternsListHelper(const Arg *arg,
+                                           SymbolPatterns &symbolPatterns) {
----------------
I would prefer to avoid naming things "helper" if there is a more descriptive name available. E.g. something like `parseSymbolPatternsFile` seems like a good fit here


================
Comment at: lld/MachO/Driver.cpp:956
+}
 static void handleSymbolPatterns(InputArgList &args,
                                  SymbolPatterns &symbolPatterns,
----------------
nit: insert line break before functions for easy vim `{` `}` movement :p


================
Comment at: lld/MachO/Driver.cpp:1418-1419
+  // takes effect.
+  // (TODO: This is kind of confusing - considering disallowing using them
+  // together for a more straightforward behaviour)
+  {
----------------
did you have a build that used both together? IMO we should prefer implementation simplicity by default in cases where ld64's behavior is questionable


================
Comment at: lld/MachO/Driver.cpp:1429
+        config->localSymbolsPresence = SymtabPresence::None;
+
+        break;
----------------
extra newline


================
Comment at: lld/MachO/SyntheticSections.cpp:990
   // files to gather them.
-  for (const InputFile *file : inputFiles) {
-    if (auto *objFile = dyn_cast<ObjFile>(file)) {
-      for (Symbol *sym : objFile->symbols) {
-        if (auto *defined = dyn_cast_or_null<Defined>(sym)) {
-          if (defined->isExternal() || !defined->isLive() ||
-              !defined->includeInSymtab)
-            continue;
-          addSymbol(localSymbols, sym);
+  // But if `-x` is set, then we don't need to.
+  if (config->localSymbolsPresence != SymtabPresence::None) {
----------------
could state explicitly that `localSymbolsHandler` will do the right thing regardless, but that this is a perf optimization


================
Comment at: lld/MachO/SyntheticSections.cpp:1018-1019
       assert(defined->isExternal());
       if (defined->privateExtern)
-        addSymbol(localSymbols, defined);
+        localSymbolsHandler(defined);
       else
----------------
I think we lack a test case for this private extern code path


================
Comment at: lld/MachO/SyntheticSections.cpp:971
+    break;
+  case SymtabPresence::None:
+    localSymbolsHandler = [&](Symbol *) { /* Do nothing*/ };
----------------
thevinster wrote:
> oontvoo wrote:
> > thevinster wrote:
> > > Does this case need to be enumerated if it's already being filtered out below? 
> > >  Does this case need to be enumerated if it's already being filtered out below?
> > 
> > Yes, because we don't need to do the check on line 1009 and line 1019. We only do that check on line 991+ is because iterating through all the inputfiles and their symbols is expensive - hence if we can avoid it (ie. when -x is set), then we should
> > 
> > 
> Whoops missed that part. Thanks for clarifying
seems worthwhile to explain this in a comment (see below)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126046