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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In https://reviews.llvm.org/D50589#1197922, @rupprecht wrote:

> 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.


I think I wasn't explicit enough with my wording to explain what I was thinking: what happens if you specify `--globalize-symbol a --keep-global-symbol b`? It looks like the code will turn a into STB_LOCAL, and b will remain as it currently is (similar points about --localize|weaken-symbol also apply). Does this match GNU's behaviour? I'd expect a to be STB_GLOBAL and b to remain unchanged.



================
Comment at: test/tools/llvm-objcopy/keep-global-symbols.test:8
+# Global1/Global2: kept via -G/--keep-global-symbol flag
+# Global3/Global4: kept via --keep-global-symbols file
+# Files exclude comments and trim whitespace.
----------------
You should probably expand this comment for the new local and weak symbols, as well as Global6.


================
Comment at: tools/llvm-objcopy/ObjcopyOpts.td:82
+      MetaVarName<"symbol">,
+      HelpText<"Keep only <symbol> as global. May be repeated to keep several "
+               "symbols as global.">;
----------------
Since this doesn't make things STB_GLOBAL, and makes things STB_WEAK remain as weak, I'd actually rephrase the help text to be something along the lines of "Convert all symbols except <symbol> to STB_LOCAL".


================
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.">;
----------------
rupprecht wrote:
> 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?
I think it's reasonable for two reasons:
1) It is possible using asm at least to create symbol names with whitespace e.g. the following will create a global symbol called "a b":
```
.global "a b"
"a b":
	nop
```
In fact, you can even get " a b " as a valid symbol name like that. Maybe we should just not trim whitespace, although I'm not confident about that.

We should probably test that this is kept in fact by our new behaviour, since by my understanding of what you are saying, it is impossible to specify such a name in the keep-global-symbols file.

2. Users who have been seeing and ignoring the warning deserve what's coming to them...


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:384
+      // --globalize-symbol: promote a symbol to global
+      // --keep-global-symbol: *only* these symbols should be kept global
+      //
----------------
Like the help text, the second bit here should probably be updated to explain what happens to symbols that aren't kept.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:700
 
+static void AddGlobalSymbolsFromFile(std::vector<std::string> &Symbols,
+                                     StringRef Filename) {
----------------
rupprecht wrote:
> 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.
Yeah, this was a mistake early on in development, and we're trying to fix it. Thanks for the NFC commit in advance.


Repository:
  rL LLVM

https://reviews.llvm.org/D50589