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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

We're basically there (for now). Just some test tidy-ups/clarifications remaining.



================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:55
+
+--- !XCOFF
+FileHeader:
----------------
I couldn't find a symbol that fits this test case: "Symbol that is not one of the three previous types", i.e. that isn't text, data, or bss. Please add a comment/rename the symbol to highlight it.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:119
+  - Name:            .func
+    Section:         .text
+    Type:            0x20
----------------
I might have missed it, but I think this is the only .text symbol? I believe you want a .text symbol that is actually exported (to show that such symbols can be exported). It's okay for this to be combined with another property that is being tested (e.g. protected visibility).


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:195-196
+## Hidden visibility.
+    Type:            0x2000
+    StorageClass:    C_HIDEXT
+    AuxEntries:
----------------
I noticed that the "StorageClass" is different between this and the "Exported" case. If you changed it to `C_EXT`, would the symbol be exported? I'm assuming that the "Type" field is the one that defines visibility here...

More generally, for the cases where the symbol isn't printed, if possible there should be exactly one thing that would need changing to cause it to be printed. This goes for every case (hidden, internal, empty name etc).


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:220-221
+       StorageMappingClass:    XMC_TC
+  - Name:            var_extern
+    SectionIndex:    0
+    AuxEntries:
----------------
I think this is the "symbol not in a section case?" If so, perhaps name it more explicitly, and/or add a comment.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:226
+       StorageMappingClass:    XMC_UA
+  - Name:            __023
+    Section:         .data
----------------
Probably should have a comment to highlight what is significant about this case.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:235
+       SectionOrLength:        0x4
+  - Name:            023__
+    Section:         .data
----------------
Probably should have a comment to highlight what is significant about this case.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:244
+       SectionOrLength:        0x4
+  - Name:            ____
+    Section:         .data
----------------
Probably should have a comment to highlight what is significant about this case.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:253
+       SectionOrLength:        0x4
+  - Name:            __02er02__
+    Section:         .data
----------------
Probably should have a comment to highlight what is significant about this case.


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1661
+
+  for (const SymbolRef &Sym : XCOFFObj->symbols()) {
+    // There is no visibility in old 32 bit XCOFF object file interpret.
----------------
`SymbolRef` is designed for lightweight copying, like `StringRef`, so no need for `const &`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735