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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin marked 7 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:55
+
+--- !XCOFF
+FileHeader:
----------------
jhenderson wrote:
> 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.
the .debug in the section .debug which is not the  text, data, or bss. 

The 

   Name:            debug
    Section:         .debug

I think symbol name 'debug" and Section Name ".debug" can explain itself. 


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:119
+  - Name:            .func
+    Section:         .text
+    Type:            0x20
----------------
jhenderson wrote:
> 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).
    Name:            .func
    Section:         .text 

means the symbol ".func" in the .text section.

   Name:  .text
  Section: .text

means the symbol ".text" in the .text section.

    Name:            .func
    Section:         .text 
 
   test 

> Do not export global symbols beginning with  "."






================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:195-196
+## Hidden visibility.
+    Type:            0x2000
+    StorageClass:    C_HIDEXT
+    AuxEntries:
----------------
jhenderson wrote:
> 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).
> 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.
 If you changed it to C_EXT, and visibility is Hidden, it not exported.  "Type" field is the one that defines visibility.

for empty Name. it is generated by the IBM compiler xlclang, which storage name always is  C_HIDEXT, always not exported. and it is no sense to export empty symbol name.

I changed StorageClass of empty symbol,  "internal_var"
, "hidden_var" to C_EXT.

and add a new test

 

```
  - Name:            hidext_var
    Section:         .data
## Protected visibility.
    Type:            0x3000
    StorageClass:    C_HIDEXT
    AuxEntries:
     - Type:                   AUX_CSECT
       SymbolAlignmentAndType: 0x09
       StorageMappingClass:    XMC_RW
```






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


================
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.
----------------
jhenderson wrote:
> `SymbolRef` is designed for lightweight copying, like `StringRef`, so no need for `const &`.
no copy is not better than any lightweight copying ?  I change as your suggestion anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735