[PATCH] D50589: [llvm-objcopy] Implement -G/--keep-global-symbol(s).

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked 5 inline comments as done.
rupprecht added a comment.

In https://reviews.llvm.org/D50589#1196944, @jhenderson wrote:

> What happens if you specify a mixture of --globalize-symbol and --keep-global-symbol?


The symbol will be a global in the output file.
That's how it accidentally was in the way I wrote it, but I added a test case to make sure it stays that way and left some more comments.



================
Comment at: test/tools/llvm-objcopy/keep-global-symbols.test:36
+  Weak:
+    - Name:        Weak
+      Section:     .text
----------------
jhenderson wrote:
> What happens if you specify -G Weak? Does it become Global? Ditto -G Local?
Added both as test cases. -G Local doesn't do anything, -G Weak prevents it from being demoted to local, and it remains a weak symbol.

The --keep-global-symbol seems subtly different from --globalize-symbol, i.e. it seems to work in the other direction. --globalize-symbol will promote a local symbol to global, whereas specifying a single --keep-global-symbol implies you want to demote all symbols to local, except for any symbols added with --keep-global-symbol.


================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:91
+          "Keep only symbols in <filename> as global. <filename> contains one "
+          "symbol per line and may contain comments beginning with '#'. May be "
+          "repeated to keep symbols from many files as global.">;
----------------
jhenderson wrote:
> What happens in GNU objcopy if you have a file containing a line like "Global1 Global2"? Does it a) keep both, b) keep neither, c) keep Global1, or d) keep a symbol called "Global1 Global2"?
It prints this amusing message and then keeps Global1:
objcopy: <path>/keep-global-symbols.test.tmp-globals2.txt:3: Ignoring rubbish found on this line.

See this part of the gnu file parser: https://github.com/redox-os/binutils-gdb/blob/a9d73bb6dacfbbdc826fea178e3a47bf7a4154b1/binutils/objcopy.c#L1076

The way I have it written, this will break compatibility -- it will trim off comments and whitespace before/after the symbol name, but not between, so it will keep a symbol called "Global1 Global2". But, I think this is a case that will never come up; anyone relying on the parsing to work like this is probably doing it wrong anyway. So, I'm inclined to leave it as written just because it's simpler. WDYT?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:700
 
+static void AddGlobalSymbolsFromFile(std::vector<std::string> &Symbols,
+                                     StringRef Filename) {
----------------
jhenderson wrote:
> Use lower case for first letter of function name.
Done -- although this is just copying the pattern from all the other static methods here which are written with an upper case first letter.
I'll go fire off an NFC to fix the names in this file.


Repository:
  rL LLVM

https://reviews.llvm.org/D50589