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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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



================
Comment at: test/tools/llvm-objcopy/keep-global-symbols.test:3
+
+# Tests that only global symbols (via -G/--keep-global-symbols).
+# Most global symbols are kept as global, only Global5 is downgraded to local.
----------------
Unfinished sentence.


================
Comment at: test/tools/llvm-objcopy/keep-global-symbols.test:5
+# Most global symbols are kept as global, only Global5 is downgraded to local.
+# Global1/Global2: kept via -G flag
+# Global3/Global4: kept via --keep-global-symbols file
----------------
Nit: -G -> -G/--keep-global-symbol


================
Comment at: test/tools/llvm-objcopy/keep-global-symbols.test:10
+# RUN: echo Global2 > %t-globals1.txt
+# RUN: (echo " Global3 "; echo "Global4 # Global5"; echo "Unknown") > %t-globals2.txt
+# RUN: echo "# File with no symbols" > %t-globals3.txt
----------------
It may be a bit clearer to do:

```
# RUN: echo " Global3 " > %t-globals2.txt
# RUN: echo "Global4 # Global5" >> %t-globals2.txt
# RUN: echo "Unknown" >> %t-globals2.txt
```


================
Comment at: test/tools/llvm-objcopy/keep-global-symbols.test:36
+  Weak:
+    - Name:        Weak
+      Section:     .text
----------------
What happens if you specify -G Weak? Does it become Global? Ditto -G 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.">;
----------------
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"?


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:700
 
+static void AddGlobalSymbolsFromFile(std::vector<std::string> &Symbols,
+                                     StringRef Filename) {
----------------
Use lower case for first letter of function name.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:708
+  BufOrErr.get()->getBuffer().split(Lines, '\n');
+  for (const auto Line : Lines) {
+    // Ignore everything after '#', trim whitespace, and only add the symbol if
----------------
I'd prefer no auto here, because it confused me on first read. Also, the const I think is unnecessary, since this is a `StringRef`.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:710
+    // Ignore everything after '#', trim whitespace, and only add the symbol if
+    // it's not empty
+    auto TrimmedLine = Line.split('#').first.trim();
----------------
Nit: missing trailing full stop.


Repository:
  rL LLVM

https://reviews.llvm.org/D50589