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

[Vincent Lee via Phabricator via llvm-commits]

thevinster added inline comments.


================
Comment at: lld/MachO/Driver.cpp:1416-1417
+  // both be present.
+  // But -x can be used with either of these two, in which case, the last arg
+  // takes effect.
+  // (TODO: This is kind of confusing - considering disallowing using them
----------------
Just to check here, does ld64 also do the same here? Wondering if this specific to our implementation or inheriting behavior for ld64. I do agree it's confusing and maybe we should disallow it if no one actually does this in practice. 


================
Comment at: lld/MachO/SyntheticSections.cpp:971
+    break;
+  case SymtabPresence::None:
+    localSymbolsHandler = [&](Symbol *) { /* Do nothing*/ };
----------------
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


================
Comment at: lld/test/MachO/local-symbol-output.s:73
+
+## Test interactions with exported_symbol.
+# RUN: %lld %t/main.o -o %t/strip_all_export_one.out \
----------------
oontvoo wrote:
> thevinster wrote:
> > Can we also have a test for interactions with `-unexported_symbol` as well? Specifically, that a global doesn't get included in the symbol table?
> I'm not sure I understand the test you're proposing. Can you give an example?
>  Specifically, -x and -non_global_symbols_[no_]strip_list options don't have an effect on global symbols. So how would using them with -unexported_symbol be meaningful?
Right, -x and -non_global_symbols_[no_]strip_list don't matter to global symbols but unexported symbols can make them non-global. For example,
```
.globl _foo
.globl _bar
_foo:
  ret
_bar:
  ret
```
What I'm thinking is if `_foo` in the test case above is unexported with -x, I would to expect to see only _bar in the symbol table since -x would've stripped away _foo. 

We can also apply this to -non_global_symbols_strip_list  and non_global_symbols_no_strip_list well by checking that the unexported symbol should've been stripped and not stripped from the symbol table, respectively. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126046