[PATCH] D112735: export unique symbol list for xcoff with llvm-objdump new option "--export-unique-symbol"

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Sorry for my slow response.

My concern regarding the necessity of `SF_XCOFF_Protected` remains.
The BasicSymbolRef::Flags members are for more widely-used attributes.
An ELF/Mach-O symbol has many attributes but very few made themselves in the list.
I understand that XCOFF has protected symbols, but that is not justification putting it in the list: ELF doesn't do that.
So, I'll keep the "Request Changes" on, until this has been resolved.

IIUI objdump originated from GNU binutils. (Then other projects ported it.)
I agree that Mach-O's overloading "objdump" (a number of Mach-O specific dumping options) was a historical design mistake.
The interface is so different. It probably should just have been separately named upfront, like "llvm-otool" we have today.

This patch focuses on symbols and I assume that you may add more symbol related dumping options.
Since you appear to be (a bit) open to the tool (to add the feature) and the option names, I agree with @jhenderson that llvm-nm is more suitable.

> if we implement the option "--export-unique-symbol" in the llvm-nm. we need to modify the following code in llvm-nm.cpp
>
> modify llvm-nm: sortAndPrintSymbolList() to disable printing the "symbol address" , "symbol flag" and enable printing "symbol visibility" if there is option "--export-unique-symbol" .
> [...]

I have some but not much Mach-O code in llvm-nm. The Mach-O part of llvm-nm really needs some refactoring in my view (but nobody has motivation to do that now).
My gut feeling says that some Mach-O specific fields (NType/NSect/NDesc) in `NMSymbol` may be a design mistake.
It may be better putting the extra symbol information in a parallel table.
If the information can be easily extracted from `SymbolRef`, extracting the information when the dumper needs it (i.e. not storing them at all) may be better.
Since you are adding new functionality, I believe some better choices can be made and you don't necessarily follow its steps (copy its mistake).

> `%p/Inputs/big-archive-libtest.a`

I think @jhenderson has mentioned this many times in the past, it'd be past to use yaml2obj to generate tests and avoid precanned binaries.
If yaml2obj is not ready, that perhaps means XCOFF contributors can prioritize on making that tool mature.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735