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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:55
+
+--- !XCOFF
+FileHeader:
----------------
DiggerLin wrote:
> 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. 
I'd like a comment that says "A symbol that is neither text, nor data, nor bss." specifically, to show that it's not that the symbol is a debug symbol, but rather that it isn't one of those three categories. That way, if behaviour were to change in the future, and debug symbols became special, there wouldn't be a risk of loss of coverage (in theory).


================
Comment at: llvm/test/tools/llvm-nm/XCOFF/export-symbols.test:119
+  - Name:            .func
+    Section:         .text
+    Type:            0x20
----------------
DiggerLin wrote:
> 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  "."
> 
> 
> 
> 
When I say ".text" symbol, I mean a symbol in ".text". The current logic has `SecIter->isText()` as a possible condition for exporting. The `.func` test case hits this, but is then not exported for other reasons. You need a test case that also hits this, and is exported. Otherwise, you could delete that part of the conditional and you'd see no test failures.


================
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.
----------------
DiggerLin wrote:
> 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.
You left the `const` in, please remove it ;)

I wouldn't be surprised if the compiler optimizes away the copy anyway, since there's no modification of `Sym`. The copy is then less code, so makes it a little easier to read.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735