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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added a comment.

In D112735#3102119 <https://reviews.llvm.org/D112735#3102119>, @jhenderson wrote:

> In D112735#3100627 <https://reviews.llvm.org/D112735#3100627>, @DiggerLin wrote:
>
>> In D112735#3100383 <https://reviews.llvm.org/D112735#3100383>, @jhenderson wrote:
>>
>>> Does this option really belong in llvm-objdump? It seems to me like it's for a different tool, probably llvm-nm (which simply prints symbol lists). In particular, I don't like the "suppresses other options" bit of the help message. llvm-objdump and llvm-readobj usually just do everything that's asked of them.
>>>
>>> The additional supporting options belong in independent patches, based on the first. If you switch to using llvm-nm, you'll find that you probably won't need the --exclude-weak option, since llvm-nm already has a --no-weak option. In fact, you may be able to get away without a new option for --export-unique-symbol, by using existing llvm-nm options, such as --dynamic, I'm not sure.
>>>
>>> Please take a look at llvm-nm, and see what you think.
>>
>> Hi James,
>>
>>   if you run llvm-objdump --help
>>
>> you will see the
>>
>>   llvm-objdump MachO Specific Options: 
>>      --exports-trie         Display mach-o exported symbols
>>
>> the option "--export-unique-symbol"  do the same thing for xcoff.
>>
>>   --export-unique-symbol  Export symbol list for xcoff object file or archive 
>>
>> and if you see the source of option 
>> --macho                 Use MachO specific object file parser
>>
>> it also suppresses other non-MachO options
>>
>> in the llvm-objdump.cpp
>>
>>> static void dumpInput(StringRef file) {
>>>
>>>   // If we are using the Mach-O specific object file parser, then let it parse
>>>   // the file and process the command line options.  So the -arch flags can
>>>   // be used to select specific slices, etc.
>>>   if (MachOOpt) {
>>>     parseInputMachO(file);
>>>     return;
>>>   }
>>>   
>>>   if (ExportUniqueSymbol) {
>>>     exportSymbolInfoFromFile(file);
>>>     printExportSymboList(outs());
>>>     return;
>>>   }
>
> Yes, this is true, but llvm-objdump for Mach-O (i.e. with the `--macho` option) is almost completely unrelated to llvm-objdump for other platforms. This, in my opinion, was a bit of a historical design mistake as most of llvm-objdump, aside from this, is generic, but there may be/have been good reasons to do it, based on the comment at the start of `dumpInput`. I think we can do better with XCOFF, and I think the functionality fits better with the existing llvm-nm functionality, and will enable greater code reuse. Note also that the use of the `--exports-trie` option does not suppress the use of other Mach-O options, like `--bind` or `--rpaths`, so your current proposal differs in this regards (`--macho` is not a normal option: it basically tells llvm-objdump to run completely differently, rather than just enabling specific dumps like most switches).



In D112735#3102119 <https://reviews.llvm.org/D112735#3102119>, @jhenderson wrote:

> In D112735#3100627 <https://reviews.llvm.org/D112735#3100627>, @DiggerLin wrote:
>
>> In D112735#3100383 <https://reviews.llvm.org/D112735#3100383>, @jhenderson wrote:
>>
>>> Does this option really belong in llvm-objdump? It seems to me like it's for a different tool, probably llvm-nm (which simply prints symbol lists). In particular, I don't like the "suppresses other options" bit of the help message. llvm-objdump and llvm-readobj usually just do everything that's asked of them.
>>>
>>> The additional supporting options belong in independent patches, based on the first. If you switch to using llvm-nm, you'll find that you probably won't need the --exclude-weak option, since llvm-nm already has a --no-weak option. In fact, you may be able to get away without a new option for --export-unique-symbol, by using existing llvm-nm options, such as --dynamic, I'm not sure.
>>>
>>> Please take a look at llvm-nm, and see what you think.
>>
>> Hi James,
>>
>>   if you run llvm-objdump --help
>>
>> you will see the
>>
>>   llvm-objdump MachO Specific Options: 
>>      --exports-trie         Display mach-o exported symbols
>>
>> the option "--export-unique-symbol"  do the same thing for xcoff.
>>
>>   --export-unique-symbol  Export symbol list for xcoff object file or archive 
>>
>> and if you see the source of option 
>> --macho                 Use MachO specific object file parser
>>
>> it also suppresses other non-MachO options
>>
>> in the llvm-objdump.cpp
>>
>>> static void dumpInput(StringRef file) {
>>>
>>>   // If we are using the Mach-O specific object file parser, then let it parse
>>>   // the file and process the command line options.  So the -arch flags can
>>>   // be used to select specific slices, etc.
>>>   if (MachOOpt) {
>>>     parseInputMachO(file);
>>>     return;
>>>   }
>>>   
>>>   if (ExportUniqueSymbol) {
>>>     exportSymbolInfoFromFile(file);
>>>     printExportSymboList(outs());
>>>     return;
>>>   }
>
> Yes, this is true, but llvm-objdump for Mach-O (i.e. with the `--macho` option) is almost completely unrelated to llvm-objdump for other platforms. This, in my opinion, was a bit of a historical design mistake as most of llvm-objdump, aside from this, is generic, but there may be/have been good reasons to do it, based on the comment at the start of `dumpInput`. I think we can do better with XCOFF, and I think the functionality fits better with the existing llvm-nm functionality, and will enable greater code reuse.

  There are XCOFFDump.cpp(COFFDump.cpp, MachODump.cpp) in the llvm-objdump framework, we can put different object format related implement in the XXXDump.cpp, if I implement the "--export-unique-symbol" option in the llvm-nm, It is not a good ideal to put the "--export-unique-symbol" in the llvm-nm.cpp and it is not a  good idea to create  a separate  the XCOFFDump.cpp for the llvm-nm in the llvm-nm directory.

> Note also that the use of the `--exports-trie` option does not suppress the use of other Mach-O options, like `--bind` or `--rpaths`, so your current proposal differs in this regards (`--macho` is not a normal option: it basically tells llvm-objdump to run completely differently, rather than just enabling specific dumps like most switches).

OK,  I agree with you, I can  implement  the "--export-unique-symbol" without suppress other options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112735