[PATCH] D112735: export unique symbol list with llvm-nm new option "--export-symbols"

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I've done a partial review of the latest update, but I'm currently quite exhausted and unable to focus enough to give you a further review.



================
Comment at: llvm/docs/CommandGuide/llvm-nm.rst:147
+
+ Print sorted symbol with their visibility (if applicable), with duplicates
+ removed.
----------------



================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-sym-list-with-invalid-section-index.test:1
+## Test the behavior of the symbol reference section
+
----------------
The test should also be named `export-symbols-with-invalid-section-index.test`.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-sym-list.test:1
+## Test the option "--export-symbols" of llvm-nm.
+## The option merge all the output of input files,sort and print out unique symbols from object file.
----------------
Test should be named `export-symbols.test`.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-sym-list.test:2
+## Test the option "--export-symbols" of llvm-nm.
+## The option merge all the output of input files,sort and print out unique symbols from object file.
+
----------------



================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-sym-list.test:16
+
+## Show that only unique symbols (with the same name and visibility) are exported.
+## RUN: llvm-nm --export-symbols %t1.o %t2.o | FileCheck --check-prefixes=COMMON,WEAK,OBJ2 %s --implicit-check-not={{.}}
----------------
Sorry, missed this. The bit in parentheses doesn't make sense, as we're talking about unique symbols, so in those cases, "same" isn't the right word.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-sym-list.test:43
+# RUN: yaml2obj -DFLAG=0x2000 --docnum=2 %s -o %t_shared.o
+
+# RUN: llvm-nm --export-symbols %t_shared.o | FileCheck %s --allow-empty --implicit-check-not={{.}}
----------------
I'd delete this blank line, since the two lines either side are closely linked.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-sym-list.test:44
+
+# RUN: llvm-nm --export-symbols %t_shared.o | FileCheck %s --allow-empty --implicit-check-not={{.}}
+
----------------
Use `count 0` rather than `--allow-empty` etc.


================
Comment at: llvm/test/tools/llvm-nm/bitcode-export-sym.test:1
+## Test the option "--export-symbols" of llvm-nm for bitcode object file.
+
----------------
I'm not sure if bitcode files are "object" files.


================
Comment at: llvm/test/tools/llvm-nm/bitcode-export-sym.test:3
+
+# RUN: echo "target triple = \"powerpcle-unknown-linux-gnu\"" > %t32.ll
+# RUN: echo "@C32 = dso_local global i32 5, align 4" >> %t32.ll
----------------
As @MaskRay has suggested in another patch, use `split-file` rather than `echo` to generate the input.


================
Comment at: llvm/tools/llvm-nm/Opts.td:20
 def dynamic : FF<"dynamic", "Display dynamic symbols instead of normal symbols">;
+def export_symbols : FF<"export-symbols", "Export symbol list for object files or archives">;
 def extern_only : FF<"extern-only", "Show only external symbols">;
----------------
Could "for object files and archives" just be "for all inputs"?


================
Comment at: llvm/tools/llvm-nm/Opts.td:55
+
+def no_rsrc : FF<"no-rsrc", "Exclude the rsrc symbol from export symbol list"
+           "for xcoff (requires --export-symbols)">, Group<grp_xcoff_o>;
----------------



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1751
+    section_iterator SecIter = *SymSecOrErr;
+    // If the symbol is not in text or data section, it is not exported.
+    if (SecIter == XCOFFObj->section_end())
----------------



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1836-1837
+    if (ExportSymbols && Obj.isXCOFF()) {
+      XCOFFObjectFile *XCOFFObj = dyn_cast<XCOFFObjectFile>(&Obj);
+      assert(XCOFFObj && "Not an XCOFFObjectFile.");
+      exportSymbolsForXCOFF(XCOFFObj, ArchiveName);
----------------
As @MaskRay said before, you can use `cast` to get the assertion automatically.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:688
+static void removeAdjacentDuplicates() {
+  if (SymbolList.size() == 0)
+    return;
----------------
MaskRay wrote:
> The whole function can be replaced with `llvm::unique` (see std::unique).
> 
> You can create an `operator==` comparator from the existing `operator<` comparator.
On that note, could we add `operator<` to the `NMSymbol` class? This would save the need for explicit predicates.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735