export unique symbol list with llvm-nm new option "--export-symbols"

[DiggerLin]

Summary

the patch implement of following functionality.

1. export the symbols from archive or object files.
2. sort the export symbols. (based on same symbol name and visibility)
3. delete the duplicate export symbols (based on same symbol name and visibility)
4. print out the unique and sorted export symbols (print the symbol name and visibility).

there are two new options are add in the patch

1. --export-symbols (enable the functionality of export unique symbol)
2. --no-rsrc (exclude the symbol name begin with "__rsrc" from be exporting from xcoff object file)

Export symbol list for xcoff object file has the same functionality as
The patch has the same functionality as
IBM CreateExportList

[MaskRay]

llvm/tools/llvm-nm/llvm-nm.cpp

723 ↗

(On Diff #401385)

char is more efficient and makes the linked output smaller

[MaskRay]

llvm/docs/CommandGuide/llvm-nm.rst

278 ↗

(On Diff #401385)

Is this option from AIX nm? Or you just think it useful?

[DiggerLin]

support --export-symbol for llvm bitcode objectfile etc.

[DiggerLin]

llvm/docs/CommandGuide/llvm-nm.rst

278 ↗

(On Diff #401385)

yes, the option is for aix only.

llvm/tools/llvm-nm/llvm-nm.cpp

270 ↗

(On Diff #401385)

yes, in the aix, it is possible that same name have different visibilities in different object files of archive file.

1694 ↗

(On Diff #401385)

yes, in aix , we skip export symbol list from shared objects.

1755 ↗

(On Diff #401385)

this is not std::Regex, it is llvm specific Regex, and the code is easy to understand, if I write my own code, the reviewer maybe take some time to understand.
but I changed as suggestion anyway.

[jhenderson]

I've done a partial review of the latest update, but I'm currently quite exhausted and unable to focus enough to give you a further review.

llvm/docs/CommandGuide/llvm-nm.rst

147 ↗

(On Diff #405078)

llvm/test/tools/llvm-nm/XCOFF/export-sym-list-with-invalid-section-index.test

1 ↗

(On Diff #405078)

The test should also be named export-symbols-with-invalid-section-index.test.

llvm/test/tools/llvm-nm/XCOFF/export-sym-list.test

1 ↗

(On Diff #405078)

Test should be named export-symbols.test.

2 ↗

(On Diff #405078)

16 ↗

(On Diff #405078)

Sorry, missed this. The bit in parentheses doesn't make sense, as we're talking about unique symbols, so in those cases, "same" isn't the right word.

43 ↗

(On Diff #405078)

I'd delete this blank line, since the two lines either side are closely linked.

44 ↗

(On Diff #405078)

Use count 0 rather than --allow-empty etc.

llvm/test/tools/llvm-nm/bitcode-export-sym.test

1 ↗

(On Diff #405078)

I'm not sure if bitcode files are "object" files.

3 ↗

(On Diff #405078)

As @MaskRay has suggested in another patch, use split-file rather than echo to generate the input.

llvm/tools/llvm-nm/Opts.td

20 ↗

(On Diff #405078)

Could "for object files and archives" just be "for all inputs"?

55 ↗

(On Diff #405078)

llvm/tools/llvm-nm/llvm-nm.cpp

688 ↗

(On Diff #401385)

On that note, could we add operator< to the NMSymbol class? This would save the need for explicit predicates.

1751 ↗

(On Diff #405078)

1836–1837 ↗

(On Diff #405078)

As @MaskRay said before, you can use cast to get the assertion automatically.

[DiggerLin]

In D112735#3289592, @jhenderson wrote:

I've done a partial review of the latest update, but I'm currently quite exhausted and unable to focus enough to give you a further review.

Thanks a lot for your time and your professional review. I can update the patch based on the partial review.

llvm/test/tools/llvm-nm/bitcode-export-sym.test

1 ↗

(On Diff #405078)

for the source code, llvm bitcode is IRObjectFile.

llvm/tools/llvm-nm/Opts.td

20 ↗

(On Diff #405078)

there are MachOUniversalBinary and TapiUniversal, using "for all inputs" is more reasonable. thanks

55 ↗

(On Diff #405078)

it only exclude the "__rsrc" symbol from the export symbol list.

[jhenderson]

As mentioned a bit inline, I think you should do some of the refactoring in separate patches, to simplify the number of changes being added here.

1. Pull out the isSymbolDefined code as you've already done.
2. Split out/update the symbol sorting code as you've done/as I've suggested in the inline comments in this patch.

I think you should also do a refactor whereby SymbolList is no longer a global variable - having it as a global variable is confusing, at best, and at worst risks cases where it is still populated when it shouldn't be.

Finally, I think having some printing happen before the symbols have been gathered may be a bad idea. At the moment, things like the input filename are printed quite early on, which isn't what we want in some cases. There's no reason this couldn't be deferred until the printing stage, so I'd look at a refactor that does this.

At the moment, the regular behaviour is essentially:

for each input file (and archive member):
    get all symbols
    sort them
    print them according to output format, filtering according to options

A possible additional refactor to the existing code would be to move filtering to the "getting" point, so that they are never added to the symbol list in the first place.

For this patch, I think you essentially want to reuse the getting, sorting and filtering stages. The difference is that rather than print then throw away the symbol list, you want to keep it and do additional stuff to it. In essence, the structure for export symbol list is:

for each input file (and archive member):
  add the symbols to a single global list
sort the full list
uniquify the full list
filter the full list
print the full list

Does this match your understanding? You'll notice that most of these steps, and the general structure are pretty similar. Consequently, I think we can restructure your new code a bit more logically. At the end of main, instead of where we currently have the llvm::for_each to dump symbol names, and then a separate if, you do something like the following:

if (ExportSymbols)
  printExportSymbolList();
else
  llvm::for_each(InputFilenames, dumpSymbolNamesFromFile);

dumpSymbolNamesFromFile (or more precisely dumpSymbolNamesFromObject) will then call a function (created by moving code out of dumpSymbolNamesFromObject) which gets the symbols and adds them to the symbol list. Let's call this getSymbolsFromObject for our purposes here. Beyond that, the process remains essentially the same. printExportSymbolList loops over all inputs and calls getSymbolsFromObject on each, storing the resultant symbols in a single combined list. It then sorts and merges the list before printing. The merging and printing is unique to this code path. The gathering, filtering and sorting is shared code with the regular code path.

Does that make sense? This isn't too far from the current implementation, but there are some subtle differences that should stop the need for lots of if(ExportSymbols) type checks (the only one necessary should be the one in main).

llvm/docs/CommandGuide/llvm-nm.rst

280–281 ↗

(On Diff #405324)

Another suggested rewording - you're now in "XCOFF-specific", so don't need to mention "for XCOFF...". Also, this option will apply to multiple inputs as much as one single one, so use the plural.

That being said, I don't think this option should be restricted to the export symbol list, and instead should always apply. This will make it similar to options like --no-weak, which makes implementation more natural.

llvm/test/tools/llvm-nm/Inputs/bitcode-sym64.ll

13 ↗

(On Diff #405324)

Nit: delete extra trailing blank line.

llvm/test/tools/llvm-nm/XCOFF/export-symbols.test

1 ↗

(On Diff #405324)

Sorry, another suggested rewording.

10 ↗

(On Diff #405324)

Is it supposed to explicitly be "\_\_tf1" and "\_\_tf9" only, or should this include e.g. \_\_tf2, \_\_tf3 etc?

If it should include, I'd reword this comment. If it shouldn't, I'd add an additional symbol called __tf2... to show that the prefix isn't removed in that case.

16 ↗

(On Diff #405324)

llvm/tools/llvm-nm/Opts.td

55–56 ↗

(On Diff #405324)

(and reflow as appropriate)

Explanation is the same as the suggested change in the CommandGuide.

llvm/tools/llvm-nm/llvm-nm.cpp

235 ↗

(On Diff #405324)

The addition of this new function is a useful improvement, but can be its own prerequisite patch. Please split it out and rebase this patch on top of that one.

243 ↗

(On Diff #405324)

Sorry, didn't think through my comment from yesterday enough. Make this an out-of-class operator, i.e.

bool operator<(const NMSymbol &A, const NMSymbol &B) { ... }

Then implement the equality operator simply as follows:

bool operator==(const NMSymbol &A, const NMSymbol &B) {
  return !(A < B) && !(B < A);
}

680 ↗

(On Diff #405324)

Consider implementing operator> as follows:

bool operator!=(const NMSymbol &A, const NMSymbol &B) {
  return !(A == B);
}

bool operator>(const NMSymbol &A, const NMSymbol &B) {
  return A != B && !(A < B);
}

and then using that directly as the predicate without the need for the lamdba:

llvm::stable_sort(SymbolList, operator>);

686 ↗

(On Diff #405324)

Let's make this a member function of NMSymbol.

697 ↗

(On Diff #405324)

We don't usually bother const-ifying local variables that aren't pointers/references.

It's not clear to me what this function name is trying to communicate. I'm guessing it's "should skip printing this symbol" so I'd rename it shouldSkipPrintingSymbol. I'd also consider, if it makes sense, making this a member function of NMSymbol, rather than passing in the flags (this only applies if all references use the NMSymbol flags member).

710 ↗

(On Diff #405324)

Spell out the type, rather than using auto here.

713 ↗

(On Diff #405324)

Should this be demangled?

1733–1737 ↗

(On Diff #405324)

As per MaskRay's comment above.

1792 ↗

(On Diff #405324)

Please don't start blocks with a blank line.

[DiggerLin]

address comment

[DiggerLin]

In D112735#3292985, @jhenderson wrote:

As mentioned a bit inline, I think you should do some of the refactoring in separate patches, to simplify the number of changes being added here.

1. Pull out the isSymbolDefined code as you've already done.
2. Split out/update the symbol sorting code as you've done/as I've suggested in the inline comments in this patch.

I think it is too late to create a separate refactor patch now ,we already have done several review on it.  the code is already in the patch, I do not think it is a good idea  to spend time to create a separate refactor patch and rebase current patch now.

I think you should also do a refactor whereby SymbolList is no longer a global variable - having it as a global variable is confusing, at best, and at worst risks cases where it is still populated when it shouldn't be.

for the export list, I think we need a global variable , otherwise we need a variable in main() , and then pass into each function.  The llvm-nm is c style code, I do not think we can do all the refactor in this patch.   if you have have good idea, please let me know, I will create  a separate refactor patch after current patch land.

Finally, I think having some printing happen before the symbols have been gathered may be a bad idea. At the moment, things like the input filename are printed quite early on, which isn't what we want in some cases. There's no reason this couldn't be deferred until the printing stage, so I'd look at a refactor that does this.

At the moment, the regular behavior is essentially:

for each input file (and archive member):
    get all symbols
    sort them
    print them according to output format, filtering according to options

A possible additional refactor to the existing code would be to move filtering to the "getting" point, so that they are never added to the symbol list in the first place.

Agree with you, we should do filter when get the all the symbols a separate refactor patch after the patch landed. there are several functions need to change include function dumpSymbolsFromDLInfoMachO()

For this patch, I think you essentially want to reuse the getting, sorting and filtering stages. The difference is that rather than print then throw away the symbol list, you want to keep it and do additional stuff to it. In essence, the structure for export symbol list is:

for each input file (and archive member):
  add the symbols to a single global list
sort the full list
uniquify the full list
filter the full list
print the full list

Does this match your understanding?

Yes.

You'll notice that most of these steps, and the general structure are pretty similar. Consequently, I think we can restructure your new code a bit more logically. At the end of main, instead of where we currently have the llvm::for_each to dump symbol names, and then a separate if, you do something like the following:

if (ExportSymbols)
  printExportSymbolList();
else
  llvm::for_each(InputFilenames, dumpSymbolNamesFromFile);

dumpSymbolNamesFromFile (or more precisely dumpSymbolNamesFromObject) will then call a function (created by moving code out of dumpSymbolNamesFromObject) which gets the symbols and adds them to the symbol list. Let's call this getSymbolsFromObject for our purposes here. Beyond that, the process remains essentially the same. printExportSymbolList loops over all inputs and calls getSymbolsFromObject on each, storing the resultant symbols in a single combined list. It then sorts and merges the list before printing. The merging and printing is unique to this code path. The gathering, filtering and sorting is shared code with the regular code path.

Does that make sense? This isn't too far from the current implementation, but there are some subtle differences that should stop the need for lots of if(ExportSymbols) type checks (the only one necessary should be the one in main).

1. Actually , there is only two additional if(ExportSymbols) in the dumpSymbolNamesFromObject() , I do not think it is big problem.
2. printExportSymbolList loops over all inputs and calls getSymbolsFromObject , I think it need to share the code the dumpSymbolNamesFromFile. and we need to added a additional if(ExportSymbols) in the dumpSymbolNamesFromFile to go to different function .

So I do think there is benefit.

llvm/docs/CommandGuide/llvm-nm.rst

280–281 ↗

(On Diff #405324)

thanks, There maybe have multi "__rsrc" symbols but has one export symbol list.

in the aix nm tools, it do not have the similar option "--no-rsrc" .
only in IBM CreateExportList has the option.
name

llvm/test/tools/llvm-nm/XCOFF/export-symbols.test

10 ↗

(On Diff #405324)

1. it is only "tf1....." and "tf9...."
2. for "__tf1...." remove the first 6 char prefix
   1. for "__tf9..." remove the first 15 char prefix.

the shell script of IBM original CreateExportList , $NF is symbol name.

....

if (substr ($NF, 1, 7) != "__sinit" &&

    substr ($NF, 1, 7) != "__sterm" &&
    match($NF,/^__[0-9]+__/)==0 ) {
   if (substr ($NF, 1, 5) == "__tf1")
                            print (substr ($NF, 7)) VISIBILITY
   else if (substr ($NF, 1, 5) == "__tf9")
                            print (substr ($NF, 15)) VISIBILITY
   else if (!('$incl_resource' && $NF == "__rsrc"))
                            print $NF VISIBILITY
}

....

llvm/tools/llvm-nm/llvm-nm.cpp

243 ↗

(On Diff #405324)

1. I just wonder what is benefit to change to out-of-class operator ? the benefit is we can use llvm::stable_sort(SymbolList, operator>); ?

since I do not think it is effecient to implement

bool operator>(const NMSymbol &A, const NMSymbol &B) {
  return A != B && !(A < B);
}

I explain below.

2 .

bool operator==(const NMSymbol &A, const NMSymbol &B) {
  return !(A < B) && !(B < A);
}

the code is not efficient, for example , when we compare to string equal,

str1==str2 is more efficient than !(str1 < str2) && !(str2 <str1)

using str1 == str2 only go through each byte of the string once,
but
!(str1 < str2) && !(str2 <str1)
need go through each byte of the string twice.

680 ↗

(On Diff #405324)

same reason
bool operator>(const NMSymbol &A, const NMSymbol &B) {

return A != B && !(A < B);

}

this need to go through each bytes twice , not efficient.

686 ↗

(On Diff #405324)

thanks

697 ↗

(On Diff #405324)

yes, the purpose of the function is "should skip printing this symbol" , thanks I will change the name , and I will it as member function.

713 ↗

(On Diff #405324)

As I know , it do not need demangle for create export list now, if it need later, we can create a new patch for it later.

[jhenderson]

In D112735#3294424, @DiggerLin wrote:

In D112735#3292985, @jhenderson wrote:

As mentioned a bit inline, I think you should do some of the refactoring in separate patches, to simplify the number of changes being added here.

1. Pull out the isSymbolDefined code as you've already done.
2. Split out/update the symbol sorting code as you've done/as I've suggested in the inline comments in this patch.

I think it is too late to create a separate refactor patch now ,we already have done several review on it.  the code is already in the patch, I do not think it is a good idea  to spend time to create a separate refactor patch and rebase current patch now.

It is never too late to split up a patch. I am not happy with this patch in its current state, plus commits should be kept as small as practical. That way, if there is a problem, it's easier to identify where the problem might be, plus it makes reviewing of individual bits of it significantly simpler. Remember, it .

I think you should also do a refactor whereby SymbolList is no longer a global variable - having it as a global variable is confusing, at best, and at worst risks cases where it is still populated when it shouldn't be.

for the export list, I think we need a global variable , otherwise we need a variable in main() , and then pass into each function.  The llvm-nm is c style code, I do not think we can do all the refactor in this patch.   if you have have good idea, please let me know, I will create  a separate refactor patch after current patch land.

My suggested design I already provided in my previous comment actively needs to avoid the global variable - the proposed getSymbolsFromObject function would return this list for use by the respective calling functions. Global variables make code re-use harder, and lead to temporal dependencies, both of which this patch is inflicted with, in part due to the global SymbolList. Doing the refactor up-front would significantly improve this patch.

Finally, I think having some printing happen before the symbols have been gathered may be a bad idea. At the moment, things like the input filename are printed quite early on, which isn't what we want in some cases. There's no reason this couldn't be deferred until the printing stage, so I'd look at a refactor that does this.

At the moment, the regular behavior is essentially:

for each input file (and archive member):
    get all symbols
    sort them
    print them according to output format, filtering according to options

A possible additional refactor to the existing code would be to move filtering to the "getting" point, so that they are never added to the symbol list in the first place.

Agree with you, we should do filter when get the all the symbols a separate refactor patch after the patch landed. there are several functions need to change include function dumpSymbolsFromDLInfoMachO()

Doing this one later is fine - I don't think it makes much difference whether filtering is done up-front or on-printing in this case.

You'll notice that most of these steps, and the general structure are pretty similar. Consequently, I think we can restructure your new code a bit more logically. At the end of main, instead of where we currently have the llvm::for_each to dump symbol names, and then a separate if, you do something like the following:

if (ExportSymbols)
  printExportSymbolList();
else
  llvm::for_each(InputFilenames, dumpSymbolNamesFromFile);

dumpSymbolNamesFromFile (or more precisely dumpSymbolNamesFromObject) will then call a function (created by moving code out of dumpSymbolNamesFromObject) which gets the symbols and adds them to the symbol list. Let's call this getSymbolsFromObject for our purposes here. Beyond that, the process remains essentially the same. printExportSymbolList loops over all inputs and calls getSymbolsFromObject on each, storing the resultant symbols in a single combined list. It then sorts and merges the list before printing. The merging and printing is unique to this code path. The gathering, filtering and sorting is shared code with the regular code path.

Does that make sense? This isn't too far from the current implementation, but there are some subtle differences that should stop the need for lots of if(ExportSymbols) type checks (the only one necessary should be the one in main).

1. Actually , there is only two additional if(ExportSymbols) in the dumpSymbolNamesFromObject() , I do not think it is big problem.

The specific if checks are just a symptom of a wider structural problem with this patch, which if not improved will lead to brittleness and the potential for bugs. I don't think we should allow this to happen.

2. printExportSymbolList loops over all inputs and calls getSymbolsFromObject , I think it need to share the code the dumpSymbolNamesFromFile. and we need to added a additional if(ExportSymbols) in the dumpSymbolNamesFromFile to go to different function .

I'm not sure I follow. In my proposed design, dumpSymbolNamesFromFile should never be touched by the export symbols code route, only getSymbolsFromObject.

So I do think there is benefit.

Did you mean "do not"?

llvm/test/tools/llvm-nm/XCOFF/export-symbols.test

10 ↗

(On Diff #405324)

I'd add an additional symbol called __tf2... to show that the prefix isn't removed in that case.

Please address this comment then.

llvm/tools/llvm-nm/llvm-nm.cpp

243 ↗

(On Diff #405324)

Yes, an out-of-class operator is simpler to use as a predicate for sorting and other algorithm functions.

I think you are being unnecessarily concerned about performance. Whilst it is important to be aware of performance, I think you'll find that the optimizer will ensure that there is no additional runtime cost to implementing the operators as suggested. If you still are concerned, try implementing it both ways and running some tests to compare the total time it takes to run llvm-nm in each case, when sorting a large list of symbols.

680 ↗

(On Diff #405324)

As noted above: the optimizer will likely take care of this for you.

[DiggerLin]

In D112735#3296068, @jhenderson wrote:

In D112735#3294424, @DiggerLin wrote:

In D112735#3292985, @jhenderson wrote:

As mentioned a bit inline, I think you should do some of the refactoring in separate patches, to simplify the number of changes being added here.

1. Pull out the isSymbolDefined code as you've already done.
2. Split out/update the symbol sorting code as you've done/as I've suggested in the inline comments in this patch.

I think it is too late to create a separate refactor patch now ,we already have done several review on it.  the code is already in the patch, I do not think it is a good idea  to spend time to create a separate refactor patch and rebase current patch now.

It is never too late to split up a patch. I am not happy with this patch in its current state, plus commits should be kept as small as practical. That way, if there is a problem, it's easier to identify where the problem might be, plus it makes reviewing of individual bits of it significantly simpler. Remember, it .

I created a refactor patch for above and rebase current patch based on it.

I think you should also do a refactor whereby SymbolList is no longer a global variable - having it as a global variable is confusing, at best, and at worst risks cases where it is still populated when it shouldn't be.

for the export list, I think we need a global variable , otherwise we need a variable in main() , and then pass into each function.  The llvm-nm is c style code, I do not think we can do all the refactor in this patch.   if you have have good idea, please let me know, I will create  a separate refactor patch after current patch land.

My suggested design I already provided in my previous comment actively needs to avoid the global variable - the proposed getSymbolsFromObject function would return this list for use by the respective calling functions. Global variables make code re-use harder, and lead to temporal dependencies, both of which this patch is inflicted with, in part due to the global SymbolList. Doing the refactor up-front would significantly improve this patch.

Since we have not reach an agreement on the getSymbolsFromObject() , I am prefer to  do "removing the global variable SymbolList" in a later separate refactor patch. Our team is been blocking on the "export symbol list", I understand your concern about the quality of the code,  but I do not think "removing a global variable SymbolList" is in the scope of the patch.

Finally, I think having some printing happen before the symbols have been gathered may be a bad idea. At the moment, things like the input filename are printed quite early on, which isn't what we want in some cases. There's no reason this couldn't be deferred until the printing stage, so I'd look at a refactor that does this.

At the moment, the regular behavior is essentially:

for each input file (and archive member):
    get all symbols
    sort them
    print them according to output format, filtering according to options

A possible additional refactor to the existing code would be to move filtering to the "getting" point, so that they are never added to the symbol list in the first place.

Agree with you, we should do filter when get the all the symbols a separate refactor patch after the patch landed. there are several functions need to change include function dumpSymbolsFromDLInfoMachO()

Doing this one later is fine - I don't think it makes much difference whether filtering is done up-front or on-printing in this case.

You'll notice that most of these steps, and the general structure are pretty similar. Consequently, I think we can restructure your new code a bit more logically. At the end of main, instead of where we currently have the llvm::for_each to dump symbol names, and then a separate if, you do something like the following:

if (ExportSymbols)
  printExportSymbolList();
else
  llvm::for_each(InputFilenames, dumpSymbolNamesFromFile);

dumpSymbolNamesFromFile (or more precisely dumpSymbolNamesFromObject) will then call a function (created by moving code out of dumpSymbolNamesFromObject) which gets the symbols and adds them to the symbol list. Let's call this getSymbolsFromObject for our purposes here. Beyond that, the process remains essentially the same. printExportSymbolList loops over all inputs and calls getSymbolsFromObject on each, storing the resultant symbols in a single combined list. It then sorts and merges the list before printing. The merging and printing is unique to this code path. The gathering, filtering and sorting is shared code with the regular code path.

Does that make sense? This isn't too far from the current implementation, but there are some subtle differences that should stop the need for lots of if(ExportSymbols) type checks (the only one necessary should be the one in main).

1. Actually , there is only two additional if(ExportSymbols) in the dumpSymbolNamesFromObject() , I do not think it is big problem.

The specific if checks are just a symptom of a wider structural problem with this patch, which if not improved will lead to brittleness and the potential for bugs. I don't think we should allow this to happen.

2. printExportSymbolList loops over all inputs and calls getSymbolsFromObject , I think it need to share the code the dumpSymbolNamesFromFile. and we need to added a additional if(ExportSymbols) in the dumpSymbolNamesFromFile to go to different function .

I'm not sure I follow. In my proposed design, dumpSymbolNamesFromFile should never be touched by the export symbols code route, only getSymbolsFromObject.

when printExportSymbolList loops over all inputs ,  we still need to get object files from archive or from  MachOUniversalBinary, TapiUniversal, there are 9 different place call dumpSymbolNamesFromObject() in function dumpSymbolNamesFromFile() , that means the object file can be extracted in 9 different way from the binary file based on the file type of the binary.  we need the functionality  to extract object file out from "dumpSymbolNamesFromFile" ,  so it is better to share the logic of extracting object file out binary file.  So still need the dumpSymbolNamesFromFile() for your suggestion printExportSymbolList().  and the function dumpSymbolNamesFromFile() is too large.  it should be better to create a refactor patch to split the function into several small functions after the patch. as dumpInput() in llvm-readobj.cpp and llvm-objdump.cpp

So I do think there is benefit.

Did you mean "do not"?

Sorry for the typo, yes, I mean "do not"

llvm/test/tools/llvm-nm/XCOFF/export-symbols.test

10 ↗

(On Diff #405324)

I do not think we need to add additional symbol called tf2... to show that the prefix isn't removed in that case. there is no source code related the logic which can remove the prefix of tf2... But I add the test anyway.

llvm/tools/llvm-nm/llvm-nm.cpp

680 ↗

(On Diff #405324)

-bash-4.2$ /xl_le/gsa/rtpgsa/projects/w/wyvern-environment/compilers/ppc64le/linux_leppc/clang.11.0.0/bin/clang++ -I/scratch/zhijian/llvm/src/llvm/include -I/scratch/zhijian/llvm/build/include -c llvm/tools/llvm-nm/llvm-nm.cpp -I/scratch/zhijian/llvm/build/tools/llvm-nm
llvm/tools/llvm-nm/llvm-nm.cpp:703:5: error: no matching function for call to 'stable_sort'

llvm::stable_sort(SymbolList, operator>);
^~~~~~~~~~~~~~~~~

/scratch/zhijian/llvm/src/llvm/include/llvm/ADT/STLExtras.h:1789:6: note: candidate template ignored: couldn't infer template argument 'Compare'
void stable_sort(R &&Range, Compare C) {

[DiggerLin]

created a separated refactor patch and rebase the patch on it, and address comments

[jhenderson]

In D112735#3297519, @DiggerLin wrote:

I created a refactor patch for above and rebase current patch based on it.

Thanks.

I think you should also do a refactor whereby SymbolList is no longer a global variable - having it as a global variable is confusing, at best, and at worst risks cases where it is still populated when it shouldn't be.

for the export list, I think we need a global variable , otherwise we need a variable in main() , and then pass into each function.  The llvm-nm is c style code, I do not think we can do all the refactor in this patch.   if you have have good idea, please let me know, I will create  a separate refactor patch after current patch land.

My suggested design I already provided in my previous comment actively needs to avoid the global variable - the proposed getSymbolsFromObject function would return this list for use by the respective calling functions. Global variables make code re-use harder, and lead to temporal dependencies, both of which this patch is inflicted with, in part due to the global SymbolList. Doing the refactor up-front would significantly improve this patch.

Since we have not reach an agreement on the getSymbolsFromObject() , I am prefer to  do "removing the global variable SymbolList" in a later separate refactor patch. Our team is been blocking on the "export symbol list", I understand your concern about the quality of the code,  but I do not think "removing a global variable SymbolList" is in the scope of the patch.

2. printExportSymbolList loops over all inputs and calls getSymbolsFromObject , I think it need to share the code the dumpSymbolNamesFromFile. and we need to added a additional if(ExportSymbols) in the dumpSymbolNamesFromFile to go to different function .

I'm not sure I follow. In my proposed design, dumpSymbolNamesFromFile should never be touched by the export symbols code route, only getSymbolsFromObject.

when printExportSymbolList loops over all inputs ,  we still need to get object files from archive or from  MachOUniversalBinary, TapiUniversal,  we need the functionality  in  "dumpSymbolNamesFromFile" ,  so we still need the dumpSymbolNamesFromFile() for printExportSymbolList().

If we need the object fetching code from dumpSymbolNamesFromFile then just pull that code into a separate function too... Here's what I'm thinking:

// Class for storing the binary and passing around associated properties, in case it's an object.
struct NMObject {
  std::unique_ptr<Binary> Bin; // Possibly any other members that might be necessary to store archive members.
  StringRef ArchiveName;
  StringRef ArchitectureName;
};

static std::vector<NMSymbol> getSymbolsFromDLInfoMachO(MachOObjectFile &MachO) {
  std::vector<NMSymbol> Symbols;
  // As original implementation of dumpSymbolsFromDLInfoMachO, but replace all references to SymbolList with Symbols.
  return Symbols;
}

static std::vector<NMSymbol> getSymbolsFromObject(NMObject &Obj) {
  std::vector<NMSymbol> Symbols;
  // Parts of original dumpSymbolNamesFromObject that get the symbols that are to be printed.
  // Filtering will be done later, rather than here, i.e. don't add the exportSymbolsForXCOFF call here.
  return Symbols;
}

static void dumpSymbolNamesFromObject(NMObject &Obj) {
  std::vector<NMSymbol> Symbols = getSymbolsFromObject(Obj);
  Symbols = sortSymbolList(std::move(Symbols)); // Or sort in place
  printSymbolList(Symbols, Obj, printName);
}

static std::vector<NMObject> getObjectsFromFile(StringRef InputFile) {
  std::vector<NMObject> Objects;
  /*
    Code from dumpSymbolNamesFromFile which retrieves the objects (and archive properties, if appropriate) inside a binary (may be a single object, or many).
    Emplace an NMObject in the vector for each such constructed object, instead of calling dumpSymbolNamesFromObject.
  */
  return Objects;
}

static void dumpSymbolNamesFromFile(StringRef InputFile) {
  std::vector<NMObject> Objects = getObjectsFromFile(InputFile);
  llvm::for_each(Objects, dumpSymbolNamesFromObject);
}

std::vector<NMSymbol> getExportSymbols(ArrayRef<NMSymbol> Symbols, NMObject &Obj) {
  std::vector<NMSymbol> ExportSymbols;
  if (auto *XCOFF = dyn_cast<XCOFFObjectFile>(Obj.Bin)) {
    // Do what's necessary to find the symbols appropriate for XCOFF export symbols.
  }
  // TODO: Add implementations for other objects here. Possibly warn otherwise.
  return ExportSymbols;
}

static void dumpExportSymbolList(ArrayRef<StringRef> InputFilenames) {
  std::vector<NMSymbol> Symbols;
  for(StringRef InputFile : InputFilenames) {
    std::vector<NMObject> FileObjects = getObjectsFromFile(InputFile);
    for(NMObject &Object : FileObjects) {
      std::vector<NMSymbol> ObjSymbols = getSymbolsFromObject(InputFile);
      ObjSymbols = getExportSymbols(ObjSymbols, Object);
      // Add ObjSymbols to the end of Symbols.
    }
  }
  Symbols = sortSymbolList(Symbols); // Or sort in place
  SymbolList.erase(std::unique(Symbols.begin(), Symbols.end()), Symbols.end());
  printExportSymbols(Symbols);
}

int main() {
  /* ... */
  if(ExportSymbols)
    dumpExportSymbolList(InputFilenames);
  else
    llvm::for_each(InputFilenames, dumpSymbolNamesFromFile);
}

I've not tested this structure out, but I'm pretty confident that it, likely with some small tweaks is both a) not a massive departure from the current structure (meaning we don't need to rewrite large amounts of code), and b) will resolve the structural issues with this patch.

llvm/tools/llvm-nm/llvm-nm.cpp

243 ↗

(On Diff #406068)

I'd rename this function, to clarify the intent - setSymFlag implies it's changing from one value to another, whereas initializeFlags shows that we're putting them into their proper initial state, and that it's bad if things go wrong.

254 ↗

(On Diff #406068)

It's easier to work with positives, so I'd suggest renaming this shouldPrint, and invert the logic.

285 ↗

(On Diff #406068)

702 ↗

(On Diff #406068)

Maybe to clarify intent, I'd call this printExportSymbolList.

1716–1723 ↗

(On Diff #406068)

This can be simplified, I believe as per the inline edit.

1718 ↗

(On Diff #406068)

Use llvm::isDigit

1743 ↗

(On Diff #406068)

Consider making this else if, for clarity.

1784–1788 ↗

(On Diff #406068)

This code doesn't really belong here, as it's just filtering out the export symbols. Later code will already retrieve XCOFf symbols without needing to do anything extra - we should filter that set of symbols after that point.

This block here is also a good example of why the current implementation in this patch is not a good structure. It looks like none of the code before this block is relevant. The if (DynamicSyms) block isn't relevant, becuase that's only for ELF. The if (!SegSect.empty() && MachO) block is also irrelevant, since an object cannot be both MachO and XCOFF. That also implies that if (!(MachO && DyldInfoOnly)) is always true, if we are XCOFF. Nothing after this block is relevant either, since it returns unconditionally. This shows that this function is not the place for this piece of code.

1849 ↗

(On Diff #406068)

I don't understand the need for this addition.

[DiggerLin]

rebased the code base on the https://reviews.llvm.org/D119028 [NFC] Refactor llvm-nm symbol comparing and split sorting

[DiggerLin]

In D112735#3303843, @jhenderson wrote:

In D112735#3297519, @DiggerLin wrote:

I created a refactor patch for above and rebase current patch based on it.

Thanks.

I think you should also do a refactor whereby SymbolList is no longer a global variable - having it as a global variable is confusing, at best, and at worst risks cases where it is still populated when it shouldn't be.

for the export list, I think we need a global variable , otherwise we need a variable in main() , and then pass into each function.  The llvm-nm is c style code, I do not think we can do all the refactor in this patch.   if you have have good idea, please let me know, I will create  a separate refactor patch after current patch land.

My suggested design I already provided in my previous comment actively needs to avoid the global variable - the proposed getSymbolsFromObject function would return this list for use by the respective calling functions. Global variables make code re-use harder, and lead to temporal dependencies, both of which this patch is inflicted with, in part due to the global SymbolList. Doing the refactor up-front would significantly improve this patch.

Since we have not reach an agreement on the getSymbolsFromObject() , I am prefer to  do "removing the global variable SymbolList" in a later separate refactor patch. Our team is been blocking on the "export symbol list", I understand your concern about the quality of the code,  but I do not think "removing a global variable SymbolList" is in the scope of the patch.

2. printExportSymbolList loops over all inputs and calls getSymbolsFromObject , I think it need to share the code the dumpSymbolNamesFromFile. and we need to added a additional if(ExportSymbols) in the dumpSymbolNamesFromFile to go to different function .

I'm not sure I follow. In my proposed design, dumpSymbolNamesFromFile should never be touched by the export symbols code route, only getSymbolsFromObject.

when printExportSymbolList loops over all inputs ,  we still need to get object files from archive or from  MachOUniversalBinary, TapiUniversal,  we need the functionality  in  "dumpSymbolNamesFromFile" ,  so we still need the dumpSymbolNamesFromFile() for printExportSymbolList().

If we need the object fetching code from dumpSymbolNamesFromFile then just pull that code into a separate function too... Here's what I'm thinking:

// Class for storing the binary and passing around associated properties, in case it's an object.
struct NMObject {
  std::unique_ptr<Binary> Bin; // Possibly any other members that might be necessary to store archive members.
  StringRef ArchiveName;
  StringRef ArchitectureName;
};

static std::vector<NMSymbol> getSymbolsFromDLInfoMachO(MachOObjectFile &MachO) {
  std::vector<NMSymbol> Symbols;
  // As original implementation of dumpSymbolsFromDLInfoMachO, but replace all references to SymbolList with Symbols.
  return Symbols;
}

static std::vector<NMSymbol> getSymbolsFromObject(NMObject &Obj) {
  std::vector<NMSymbol> Symbols;
  // Parts of original dumpSymbolNamesFromObject that get the symbols that are to be printed.
  // Filtering will be done later, rather than here, i.e. don't add the exportSymbolsForXCOFF call here.
  return Symbols;
}

static void dumpSymbolNamesFromObject(NMObject &Obj) {
  std::vector<NMSymbol> Symbols = getSymbolsFromObject(Obj);
  Symbols = sortSymbolList(std::move(Symbols)); // Or sort in place
  printSymbolList(Symbols, Obj, printName);
}

static std::vector<NMObject> getObjectsFromFile(StringRef InputFile) {
  std::vector<NMObject> Objects;
  /*
    Code from dumpSymbolNamesFromFile which retrieves the objects (and archive properties, if appropriate) inside a binary (may be a single object, or many).
    Emplace an NMObject in the vector for each such constructed object, instead of calling dumpSymbolNamesFromObject.
  */
  return Objects;
}

static void dumpSymbolNamesFromFile(StringRef InputFile) {
  std::vector<NMObject> Objects = getObjectsFromFile(InputFile);
  llvm::for_each(Objects, dumpSymbolNamesFromObject);
}

std::vector<NMSymbol> getExportSymbols(ArrayRef<NMSymbol> Symbols, NMObject &Obj) {
  std::vector<NMSymbol> ExportSymbols;
  if (auto *XCOFF = dyn_cast<XCOFFObjectFile>(Obj.Bin)) {
    // Do what's necessary to find the symbols appropriate for XCOFF export symbols.
  }
  // TODO: Add implementations for other objects here. Possibly warn otherwise.
  return ExportSymbols;
}

static void dumpExportSymbolList(ArrayRef<StringRef> InputFilenames) {
  std::vector<NMSymbol> Symbols;
  for(StringRef InputFile : InputFilenames) {
    std::vector<NMObject> FileObjects = getObjectsFromFile(InputFile);
    for(NMObject &Object : FileObjects) {
      std::vector<NMSymbol> ObjSymbols = getSymbolsFromObject(InputFile);
      ObjSymbols = getExportSymbols(ObjSymbols, Object);
      // Add ObjSymbols to the end of Symbols.
    }
  }
  Symbols = sortSymbolList(Symbols); // Or sort in place
  SymbolList.erase(std::unique(Symbols.begin(), Symbols.end()), Symbols.end());
  printExportSymbols(Symbols);
}

int main() {
  /* ... */
  if(ExportSymbols)
    dumpExportSymbolList(InputFilenames);
  else
    llvm::for_each(InputFilenames, dumpSymbolNamesFromFile);
}

I've not tested this structure out, but I'm pretty confident that it, likely with some small tweaks is both a) not a massive departure from the current structure (meaning we don't need to rewrite large amounts of code), and b) will resolve the structural issues with this patch.

thanks for your sample code, I agree with you, the patch is quite large now. if I implement as your suggestion in the patch , I believe that we will need another refactor patch which split the dumpSymbolNamesFromFile function into several small function before the patch. Our patch https://reviews.llvm.org/D119147 is depend on the patch. My suggestion is that I will do refactor as your suggestion after the current patch land and at least let the change code in XCOFFObjectFile.cpp , test case, llvm-nm.rst etc landed, and we can focus on refactoring the llvm-nm.cpp.

if you strong insisted that we should do as your suggestion in the patch first, I can do it and postpones our schedule.

FYI @hubert.reinterpretcast .

llvm/tools/llvm-nm/llvm-nm.cpp

243 ↗

(On Diff #406068)

thanks

254 ↗

(On Diff #406068)

thanks

285 ↗

(On Diff #406068)

thanks

1716–1723 ↗

(On Diff #406068)

This can be simplified, I believe as per the inline edit.

1716–1723 ↗

(On Diff #406068)

thanks for simplify

1784–1788 ↗

(On Diff #406068)

agree with you , but I do not think there is better choice in current structure.

1849 ↗

(On Diff #406068)

without setSymFlag(Obj). the S do not have a value SymFlags, it will crash at function
bool shouldSkipPrinting()

[DiggerLin]

In D112735#3303843, @jhenderson wrote:

In D112735#3297519, @DiggerLin wrote:

I created a refactor patch for above and rebase current patch based on it.

Thanks.

I think you should also do a refactor whereby SymbolList is no longer a global variable - having it as a global variable is confusing, at best, and at worst risks cases where it is still populated when it shouldn't be.

for the export list, I think we need a global variable , otherwise we need a variable in main() , and then pass into each function.  The llvm-nm is c style code, I do not think we can do all the refactor in this patch.   if you have have good idea, please let me know, I will create  a separate refactor patch after current patch land.

My suggested design I already provided in my previous comment actively needs to avoid the global variable - the proposed getSymbolsFromObject function would return this list for use by the respective calling functions. Global variables make code re-use harder, and lead to temporal dependencies, both of which this patch is inflicted with, in part due to the global SymbolList. Doing the refactor up-front would significantly improve this patch.

Since we have not reach an agreement on the getSymbolsFromObject() , I am prefer to  do "removing the global variable SymbolList" in a later separate refactor patch. Our team is been blocking on the "export symbol list", I understand your concern about the quality of the code,  but I do not think "removing a global variable SymbolList" is in the scope of the patch.

2. printExportSymbolList loops over all inputs and calls getSymbolsFromObject , I think it need to share the code the dumpSymbolNamesFromFile. and we need to added a additional if(ExportSymbols) in the dumpSymbolNamesFromFile to go to different function .

I'm not sure I follow. In my proposed design, dumpSymbolNamesFromFile should never be touched by the export symbols code route, only getSymbolsFromObject.

when printExportSymbolList loops over all inputs ,  we still need to get object files from archive or from  MachOUniversalBinary, TapiUniversal,  we need the functionality  in  "dumpSymbolNamesFromFile" ,  so we still need the dumpSymbolNamesFromFile() for printExportSymbolList().

If we need the object fetching code from dumpSymbolNamesFromFile then just pull that code into a separate function too... Here's what I'm thinking:

// Class for storing the binary and passing around associated properties, in case it's an object.
struct NMObject {
  std::unique_ptr<Binary> Bin; // Possibly any other members that might be necessary to store archive members.
  StringRef ArchiveName;
  StringRef ArchitectureName;
};

static std::vector<NMSymbol> getSymbolsFromDLInfoMachO(MachOObjectFile &MachO) {
  std::vector<NMSymbol> Symbols;
  // As original implementation of dumpSymbolsFromDLInfoMachO, but replace all references to SymbolList with Symbols.
  return Symbols;
}

static std::vector<NMSymbol> getSymbolsFromObject(NMObject &Obj) {
  std::vector<NMSymbol> Symbols;
  // Parts of original dumpSymbolNamesFromObject that get the symbols that are to be printed.
  // Filtering will be done later, rather than here, i.e. don't add the exportSymbolsForXCOFF call here.
  return Symbols;
}

static void dumpSymbolNamesFromObject(NMObject &Obj) {
  std::vector<NMSymbol> Symbols = getSymbolsFromObject(Obj);
  Symbols = sortSymbolList(std::move(Symbols)); // Or sort in place
  printSymbolList(Symbols, Obj, printName);
}

static std::vector<NMObject> getObjectsFromFile(StringRef InputFile) {
  std::vector<NMObject> Objects;
  /*
    Code from dumpSymbolNamesFromFile which retrieves the objects (and archive properties, if appropriate) inside a binary (may be a single object, or many).
    Emplace an NMObject in the vector for each such constructed object, instead of calling dumpSymbolNamesFromObject.
  */
  return Objects;
}

static void dumpSymbolNamesFromFile(StringRef InputFile) {
  std::vector<NMObject> Objects = getObjectsFromFile(InputFile);
  llvm::for_each(Objects, dumpSymbolNamesFromObject);
}

std::vector<NMSymbol> getExportSymbols(ArrayRef<NMSymbol> Symbols, NMObject &Obj) {
  std::vector<NMSymbol> ExportSymbols;
  if (auto *XCOFF = dyn_cast<XCOFFObjectFile>(Obj.Bin)) {
    // Do what's necessary to find the symbols appropriate for XCOFF export symbols.
  }
  // TODO: Add implementations for other objects here. Possibly warn otherwise.
  return ExportSymbols;
}

static void dumpExportSymbolList(ArrayRef<StringRef> InputFilenames) {
  std::vector<NMSymbol> Symbols;
  for(StringRef InputFile : InputFilenames) {
    std::vector<NMObject> FileObjects = getObjectsFromFile(InputFile);
    for(NMObject &Object : FileObjects) {
      std::vector<NMSymbol> ObjSymbols = getSymbolsFromObject(InputFile);
      ObjSymbols = getExportSymbols(ObjSymbols, Object);
      // Add ObjSymbols to the end of Symbols.
    }
  }
  Symbols = sortSymbolList(Symbols); // Or sort in place
  SymbolList.erase(std::unique(Symbols.begin(), Symbols.end()), Symbols.end());
  printExportSymbols(Symbols);
}

int main() {
  /* ... */
  if(ExportSymbols)
    dumpExportSymbolList(InputFilenames);
  else
    llvm::for_each(InputFilenames, dumpSymbolNamesFromFile);
}

I've not tested this structure out, but I'm pretty confident that it, likely with some small tweaks is both a) not a massive departure from the current structure (meaning we don't need to rewrite large amounts of code), and b) will resolve the structural issues with this patch.

thanks for your sample code, I agree with you, but the patch is quite large now. if I implement as your suggestion in the patch , I believe that we will need another refactor patch which split the dumpSymbolNamesFromFile function into several small function before the patch. Our patch https://reviews.llvm.org/D119147 is depend on the patch. My suggestion is that I will do refactor as your suggestion after the current patch land and at least let the change code in XCOFFObjectFile.cpp , test case, llvm-nm.rst etc which not related to the refactor landed first, and we can focus on refactoring the llvm-nm.cpp.

if you strong suggest that we should do as your suggestion in the patch first, I can do it and postpones our schedule. thanks.

FYI @hubert.reinterpretcast .

[jhenderson]

Okay, I'm willing to accept delaying further refactoring, as long as it is definitely going to happen sooner rather than later. Just a few nits to tidy up before I think you can land this patch.

llvm/test/tools/llvm-nm/XCOFF/export-symbols-with-invalid-section-index.test

1 ↗

(On Diff #406935)

Could you fold this test case into the positive test case file? Would that allow you to reuse the YAML object, using -D to provide the invalid section index?

llvm/tools/llvm-nm/llvm-nm.cpp

254 ↗

(On Diff #406068)

Just noting this hasn't been done, although you can defer to another patch if you prefer.

1784–1788 ↗

(On Diff #406068)

Let's at least in this patch pull the block outside of the if (!(MachO && DyldInfoOnly)) clause, since it doesn't need to be inside it.

1849 ↗

(On Diff #406068)

But the code didn't have this before, so why does it need it now? Is this a result of some of the refactoring work you've already done? I take it there's a concrete reproducible you could craft that would cause the crash without this change?

1658 ↗

(On Diff #406935)

Let's rename this function now: getXCOFFExports seems like a nice concise name.

[DiggerLin]

In D112735#3307226, @jhenderson wrote:

Okay, I'm willing to accept delaying further refactoring, as long as it is definitely going to happen sooner rather than later. Just a few nits to tidy up before I think you can land this patch.

Thanks, I can start the refactor from next week.

In D112735#3307226, @jhenderson wrote:

Okay, I'm willing to accept delaying further refactoring, as long as it is definitely going to happen sooner rather than later. Just a few nits to tidy up before I think you can land this patch.

Thanks a lot , I will begin to refactor as soon as possible, I think I can begin to refactor  from late of next week.

llvm/test/tools/llvm-nm/XCOFF/export-symbols-with-invalid-section-index.test

1 ↗

(On Diff #406935)

thanks.

llvm/tools/llvm-nm/llvm-nm.cpp

254 ↗

(On Diff #406068)

done ,thanks

1784–1788 ↗

(On Diff #406068)

thanks

1849 ↗

(On Diff #406068)

the function shouldSkipPrinting() is called
in the printExportSymbolList()
but the printExportSymbolList are called after dumpSymbolNamesFromFile , The object binary file maybe be freed from memory when there are several files inputs from command line. in the function dumpSymbolNamesFromFile() ,

Expected<std::unique_ptr<Binary>> BinaryOrErr =
      createBinary(BufferOrErr.get()->getMemBufferRef(), ContextPtr);

it generate a binary and freed it at the end of the function . so if we still try to get the SymFlags of symbol from bit code object file , it will be crashed.

so I set the SymFlag for getSymbolNamesFromObject()
as

if (S.initializeFlags(Obj))
  SymbolList.push_back(S);

dumpSymbolsFromDLInfoMachO（） initiate the SymFlag for SymFlags . but getSymbolNamesFromObject do not set it, for consistency， I set the SymFlag for the NMSymbol in getSymbolNamesFromObject

1658 ↗

(On Diff #406935)

thanks

[DiggerLin]

1. address comment.
2. move the functionality of "removing the symbols which should not be printed" to before the functionality of "removing duplication symbols". and add a new test for it. Name: __tf2value Section: .data Type: 0x0 StorageClass: C_HIDEXT AuxEntries:
   • Type: AUX_CSECT SymbolAlignmentAndType: 0x21 StorageMappingClass: XMC_TC

Sometime we have two symbols, which both has the same name and no visibility, one's storageClass is C_HIDEXT (the should not be export), another is C_EXT(the one should be exported).
When compare these two symbols for "--export-symbols", these two symbol are same, if we remove duplication symbol before removing the symbols which should not print. the Symbol with C_EXT maybe removed. it will cause none of the two symbol be printed out.

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/export-symbols.test

5 ↗

(On Diff #407285)

Tip: you can avoid the need for -DSECT=2 in the "regular" case, by using a default value in the YAML. I can't remember if the syntax is [[SECT=2]] or [[SECT:2]], but one of them should work (look for examples in other tests).

llvm/tools/llvm-nm/llvm-nm.cpp

1849 ↗

(On Diff #406068)

Thanks. It would be nice we can avoid this issue with the proposed refactoring (and therefore delete this new code), but if it's needed for now, so be it.

701 ↗

(On Diff #407285)

Nit: clang-format is complaining.

1668 ↗

(On Diff #407285)

Should this be a continue rather than a return? Also, should it be closer to the rest of the name checks further down in this loop?

1677–1713 ↗

(On Diff #407285)

I haven't got the time right now to check this myself, but you should ensure that you have symbols that trigger every possible code path that lead to them being skipped/not skipped.

A quick skim suggests at least the following:

1. INTERNAL visibility
2. HIDDEN visibility
3. No visibility attribute
4. Visibility attribute that isn't internal or hidden.
5. Symbol with error when looking up section
6. Symbol that is not in a section (SecIter == XCOFFObj->section_end())
7. Text symbol
8. Data symbol
9. BSS symbol
10. Symbol that is not one of the three previous types
11. Symbol with empty name
12. Symbol with name look-up failure
13. __sinit prefixed symbol
14. __sterm prefixed symbol
15. . prefixed symbol
16. ( prefixed symbol
17. ____ named symbol (i.e. four underscores, nothing between prefix and suffix)
18. __<digits>__ symbol
19. __<digits> (no suffix)
20. <digits>__ (no prefix)
21. __<digits and something non digit>__ symbol

1720–1721 ↗

(On Diff #407285)

This block appears after the name adjustments for __tf1 and __tf9. If the intent is that __tf1__rsrc and __tf9_rsrc are skipped with --no-rsrc, you need to check them. In fact, it probably doens't hurt to have that test case even if they should be kept.

1773–1777 ↗

(On Diff #407285)

I'd be tempted to move this block right to the start of the function, since the other bits are completely irrelevant if we trigger this block.

[DiggerLin]

llvm/tools/llvm-nm/llvm-nm.cpp

1668 ↗

(On Diff #407285)

thanks

1677–1713 ↗

(On Diff #407285)

I added all test cases you mention above except the "12 Symbol with name look-up failure"

from the source code in the XCOFFObjectFile.cpp

Expected<StringRef> XCOFFSymbolRef::getName() const {
  // A storage class value with the high-order bit on indicates that the name is
  // a symbolic debugger stabstring.
  if (getStorageClass() & 0x80)
    return StringRef("Unimplemented Debug Name");

  if (Entry32) {
    if (Entry32->NameInStrTbl.Magic != XCOFFSymbolRef::NAME_IN_STR_TBL_MAGIC)
      return generateXCOFFFixedNameStringRef(Entry32->SymbolName);

    return OwningObjectPtr->getStringTableEntry(Entry32->NameInStrTbl.Offset);
  }

  return OwningObjectPtr->getStringTableEntry(Entry64->Offset);
}

It never return an Error.

The code in the function exportSymbolsForXCOFF

Expected<StringRef> NameOrErr = Sym.getName();
   if (!NameOrErr) {
     warn(NameOrErr.takeError(), XCOFFObj->getFileName(),
          "for symbol with index " +
              Twine(XCOFFObj->getSymbolIndex(Sym.getRawDataRefImpl().p)),
          ArchiveName);
     continue;
   }
   StringRef SymName = *NameOrErr;

can be modified to

StringRef SymName = cantFail(Sym.getName());

but I prefer to keep as it is. The reason as:

if Expected<StringRef> XCOFFSymbolRef::getName() const is changed to return with Error in some day.

using StringRef SymName = cantFail(Sym.getName()); will cause an llvm-unreable.

1720–1721 ↗

(On Diff #407285)

good catch, thanks , the tf1_rsrc should be export out as "__rsrc" , I changed the code and add test case for it.

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/export-symbols.test

72 ↗

(On Diff #407656)

It's probably worth adding a comment to the start of each symbol block in this YAML explaining what case(s) that symbol covers.

llvm/tools/llvm-nm/llvm-nm.cpp

1677–1713 ↗

(On Diff #407285)

I think you should switch to cantFail, unless you know that you are going to change getName soon. The reasons are:

1. it simplifies the code dramatically, making it more readable.
2. you can't test the current code, so there's no guarantee that even if getName is changed that it is handled here appropriately.
3. if getName is changed to return an Error at some point, call sites should be audited for additional checks/tests that are needed. As such, this case should be picked up anyway.

[DiggerLin]

llvm/test/tools/llvm-nm/XCOFF/export-symbols.test

72 ↗

(On Diff #407656)

I think most the symbol name can express what we want to test. I only added some visibility comment on the YAML . and symbol name "var_extern" which test the

if (SecIter == XCOFFObj->section_end())
  continue;

5 ↗

(On Diff #407285)

thanks a lot

llvm/tools/llvm-nm/llvm-nm.cpp

1677–1713 ↗

(On Diff #407285)

thanks

[jhenderson]

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

llvm/test/tools/llvm-nm/XCOFF/export-symbols.test

55 ↗

(On Diff #408561)

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.

119 ↗

(On Diff #408561)

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).

195–196 ↗

(On Diff #408561)

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).

220–221 ↗

(On Diff #408561)

I think this is the "symbol not in a section case?" If so, perhaps name it more explicitly, and/or add a comment.

226 ↗

(On Diff #408561)

Probably should have a comment to highlight what is significant about this case.

235 ↗

(On Diff #408561)

Probably should have a comment to highlight what is significant about this case.

244 ↗

(On Diff #408561)

Probably should have a comment to highlight what is significant about this case.

253 ↗

(On Diff #408561)

Probably should have a comment to highlight what is significant about this case.

llvm/tools/llvm-nm/llvm-nm.cpp

1661 ↗

(On Diff #408561)

SymbolRef is designed for lightweight copying, like StringRef, so no need for const &.

[DiggerLin]

llvm/test/tools/llvm-nm/XCOFF/export-symbols.test

55 ↗

(On Diff #408561)

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.

119 ↗

(On Diff #408561)

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 "."

195–196 ↗

(On Diff #408561)

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

220–221 ↗

(On Diff #408561)

I change to undef_var, which is not in any section

llvm/tools/llvm-nm/llvm-nm.cpp

1661 ↗

(On Diff #408561)

no copy is not better than any lightweight copying ? I change as your suggestion anyway.

[jhenderson]

llvm/test/tools/llvm-nm/XCOFF/export-symbols.test

55 ↗

(On Diff #408561)

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).

119 ↗

(On Diff #408561)

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.

llvm/tools/llvm-nm/llvm-nm.cpp

1661 ↗

(On Diff #408561)

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.

[DiggerLin]

llvm/test/tools/llvm-nm/XCOFF/export-symbols.test

119 ↗

(On Diff #408561)

I changed the

Name:            __tf1_tf1value
Section:         .data

-->

Name:            __tf1_tf1value
Section:         .text

[jhenderson]

Okay, looks good from me. Best wait for @MaskRay too.

[MaskRay]

LGTM.

llvm/docs/CommandGuide/llvm-nm.rst

280 ↗

(On Diff #409252)

llvm/test/tools/llvm-nm/bitcode-export-sym.test

1 ↗

(On Diff #409252)

Add # REQUIRES: powerpc-registered-target

[DiggerLin]

llvm/docs/CommandGuide/llvm-nm.rst

280 ↗

(On Diff #409252)

thanks

llvm/test/tools/llvm-nm/bitcode-export-sym.test

1 ↗

(On Diff #409252)

I wonder why we need # REQUIRES: powerpc-registered-target here . I think the llvm is cross compile, it can generate the bit code for powerpc on other platform, I added the line anyway.

llvm/docs/CommandGuide/llvm-objdump.rst

.. option:: --universal-headers
 
   Display universal headers.
 
 .. option:: --weak-bind
 
   Display weak binding information.
 
 XCOFF ONLY OPTIONS AND COMMANDS
 ---------------------------------
 
 .. option:: --symbol-description
 
   Add symbol description to disassembly output.
 
+.. option:: --export-unique-symbol
+
+  Export symbol list for xcoff object file or archive. The option will suppress other options.
+
+.. option:: --exclude-weak
+
+  Exclude the weak symbol from export symbol list for xcoff files(requires --export-unique-symbol).
+
+.. option:: --exclude-rsrc
+
+  Exclude the rsrc symbol from export symbol list for xcoff files(requires --export-unique-symbol).  
+
+
 BUGS
 ----
 
 To report bugs, please visit <https://bugs.llvm.org/>.
 
 SEE ALSO
 --------
 
 :manpage:`llvm-nm(1)`, :manpage:`llvm-otool(1)`, :manpage:`llvm-readelf(1)`,
 :manpage:`llvm-readobj(1)`

llvm/include/llvm/BinaryFormat/XCOFF.h

 constexpr size_t RelocationSerializationSize32 = 10;
 constexpr size_t RelocationSerializationSize64 = 14;
 constexpr uint16_t RelocOverflow = 65535;
 constexpr uint8_t AllocRegNo = 31;
 
 enum ReservedSectionNum : int16_t { N_DEBUG = -2, N_ABS = -1, N_UNDEF = 0 };
 
 enum MagicNumber : uint16_t { XCOFF32 = 0x01DF, XCOFF64 = 0x01F7 };
+enum XCOFFInterpret : uint16_t {
+  OLD_XCOFF_INTERPRET = 1,
+  NEW_XCOFF_INTERPRET = 2
+};
+
+enum FileFlag : uint16_t {
+  F_RELFLG = 0x0001,    ///< relocation info stripped from file
+  F_EXEC = 0x0002,      ///< file is executable (i.e., it
+                        ///< has a loader section)
+  F_LNNO = 0x0004,      ///< line numbers stripped from file
+  F_LSYMS = 0x0008,     ///< local symbols stripped from file
+  F_FDPR_PROF = 0x0010, ///< file was profiled with FDPR
+  F_FDPR_OPTI = 0x0020, ///< file was reordered with FDPR
+  F_DSA = 0x0040,       ///< file uses Dynamic Segment Allocation (32-bit
+                        ///< only)
+  F_DEP_1 = 0x0080,     ///< data Execution Protection bit 1
+  F_VARPG = 0x0100,     ///< executable requests using variable size pages
+  F_LPTEXT = 0x0400,    ///<  executable requires large pages for text
+  F_LPDATA = 0x0800,    ///< executable requires large pages for data
+  F_DYNLOAD = 0x1000,   ///< file is dynamically loadable and
+                        ///< executable (equivalent to F_EXEC on AIX)
+  F_SHROBJ = 0x2000,    ///<  file is a shared object
+  F_LOADONLY =
+      0x4000,      ///<  file can be loaded by the system loader, but it is
+                   ///<  ignored by the linker if it is a member of an archive.
+  F_DEP_2 = 0x8000 ///<  Data Execution Protection bit 2
+};
 
 // x_smclas field of x_csect from system header: /usr/include/syms.h
 /// Storage Mapping Class definitions.
 enum StorageMappingClass : uint8_t {
   //     READ ONLY CLASSES
   XMC_PR = 0,      ///< Program Code
   XMC_RO = 1,      ///< Read Only Constant
   XMC_DB = 2,      ///< Debug Dictionary Table
   XMC_GL = 6,      ///< Global Linkage (Interfile Interface Code)
   XMC_XO = 7,      ///< Extended Operation (Pseudo Machine Instruction)
   XMC_SV = 8,      ///< Supervisor Call (32-bit process only)
   XMC_SV64 = 17,   ///< Supervisor Call for 64-bit process
   XMC_SV3264 = 18, ///< Supervisor Call for both 32- and 64-bit processes
   XMC_TI = 12,     ///< Traceback Index csect
   XMC_TB = 13,     ///< Traceback Table csect
 
   //       READ WRITE CLASSES
   XMC_RW = 5,   ///< Read Write Data
   XMC_TC0 = 15, ///< TOC Anchor for TOC Addressability
   XMC_TC = 3,   ///< General TOC item
   XMC_TD = 16,  ///< Scalar data item in the TOC
   XMC_DS = 10,  ///< Descriptor csect
   XMC_UA = 4,   ///< Unclassified - Treated as Read Write
   XMC_BS = 9,   ///< BSS class (uninitialized static internal)
   XMC_UC = 11,  ///< Un-named Fortran Common
 
   XMC_TL = 20, ///< Initialized thread-local variable
   XMC_UL = 21, ///< Uninitialized thread-local variable
   XMC_TE = 22  ///< Symbol mapped at the end of TOC
 };
 
 // Flags for defining the section type. Masks for use with the (signed, 32-bit)
 // s_flags field of the section header structure, selecting for values in the
 // lower 16 bits. Defined in the system header `scnhdr.h`.
 enum SectionTypeFlags : int32_t {

 enum VisibilityType : uint16_t {
   SYM_V_UNSPECIFIED = 0x0000,
   SYM_V_INTERNAL = 0x1000,
   SYM_V_HIDDEN = 0x2000,
   SYM_V_PROTECTED = 0x3000,
   SYM_V_EXPORTED = 0x4000
 };
 
+static constexpr uint16_t VISIBILITY_MASK = 0x7000;
+
 // Relocation types, defined in `/usr/include/reloc.h`.
 enum RelocationType : uint8_t {
   R_POS = 0x00, ///< Positive relocation. Provides the address of the referenced
                 ///< symbol.
   R_RL = 0x0c,  ///< Positive indirect load relocation. Modifiable instruction.
   R_RLA = 0x0d, ///< Positive load address relocation. Modifiable instruction.
 
   R_NEG = 0x01, ///< Negative relocation. Provides the negative of the address

llvm/include/llvm/Object/SymbolicFile.h

enum Flags : unsigned {
     SF_Exported = 1U << 6,       // Symbol is visible to other DSOs
     SF_FormatSpecific = 1U << 7, // Specific to the object file format
                                  // (e.g. section symbols)
     SF_Thumb = 1U << 8,          // Thumb symbol in a 32-bit ARM binary
     SF_Hidden = 1U << 9,         // Symbol has hidden visibility
     SF_Const = 1U << 10,         // Symbol value is constant
     SF_Executable = 1U << 11,    // Symbol points to an executable section
                                  // (IR only)
+    SF_XCOFF_Protected =
+        1U << 12,          // Symbol has protected visibility for xcoff only
+    SF_Internal = 1U << 13 // Symbol has internal visibility
   };
 
   BasicSymbolRef() = default;
   BasicSymbolRef(DataRefImpl SymbolP, const SymbolicFile *Owner);
 
   bool operator==(const BasicSymbolRef &Other) const;
   bool operator<(const BasicSymbolRef &Other) const;
 

llvm/include/llvm/Object/XCOFFObjectFile.h

struct XCOFFFileHeader64 {
   support::ubig32_t NumberOfSymTableEntries;
 };
 
 template <typename T> struct XCOFFAuxiliaryHeader {
   static constexpr uint8_t AuxiHeaderFlagMask = 0xF0;
   static constexpr uint8_t AuxiHeaderTDataAlignmentMask = 0x0F;
 
 public:
   uint8_t getFlag() const {
     return static_cast<const T *>(this)->FlagAndTDataAlignment &
            AuxiHeaderFlagMask;
   }
   uint8_t getTDataAlignment() const {
     return static_cast<const T *>(this)->FlagAndTDataAlignment &
            AuxiHeaderTDataAlignmentMask;
   }
+
+  uint16_t getVersion() const { return static_cast<const T *>(this)->Version; }
 };
 
 struct XCOFFAuxiliaryHeader32 : XCOFFAuxiliaryHeader<XCOFFAuxiliaryHeader32> {
   support::ubig16_t
       AuxMagic; ///< If the value of the o_vstamp field is greater than 1, the
                 ///< o_mflags field is reserved for future use and it should
                 ///< contain 0. Otherwise, this field is not used.
   support::ubig16_t
       Version; ///< The valid values are 1 and 2. When the o_vstamp field is 2

llvm/lib/Object/XCOFFObjectFile.cpp

if (CsectAuxEntOrErr) {
         Result |= SymbolRef::SF_Common;
     } else
       return CsectAuxEntOrErr.takeError();
   }
 
   if (XCOFFSym.getSectionNumber() == XCOFF::N_UNDEF)
     Result |= SymbolRef::SF_Undefined;
 
+  // There is no visibility in old 32 bit XCOFF object file interpret.
+  if (is64Bit() || (auxiliaryHeader32() && (auxiliaryHeader32()->getVersion() ==
+                                            NEW_XCOFF_INTERPRET))) {
+
+    uint16_t SymType = XCOFFSym.getSymbolType();
+    if ((SymType & VISIBILITY_MASK) == SYM_V_INTERNAL)
+      Result |= SymbolRef::SF_Internal;
+    if ((SymType & VISIBILITY_MASK) == SYM_V_HIDDEN)
+      Result |= SymbolRef::SF_Hidden;
+    if ((SymType & VISIBILITY_MASK) == SYM_V_PROTECTED)
+      Result |= SymbolRef::SF_XCOFF_Protected;
+    if ((SymType & VISIBILITY_MASK) == SYM_V_EXPORTED)
+      Result |= SymbolRef::SF_Exported;
+  }
   return Result;
 }
 
 basic_symbol_iterator XCOFFObjectFile::symbol_begin() const {
   DataRefImpl SymDRI;
   SymDRI.p = reinterpret_cast<uintptr_t>(SymbolTblPtr);
   return basic_symbol_iterator(SymbolRef(SymDRI, this));
 }

llvm/test/tools/llvm-objdump/XCOFF/export_sym_list_ar.test

+; Test the functionality of the llvm-objdump --export-unique-symbol  exporting  symbol list of aix archive file. 
+
+; RUN: llvm-objdump --export-unique-symbol %p/Inputs/big-archive-libtest.a | FileCheck --check-prefixes=SYM %s
+
+; The archive file big-archive-libtest.a is created from test_xclang++.o and test1_gcc.o with command 
+; ar -v -q big-archive-libtest.a test_xclang++.o test1_gcc.o 
+
+; test_xclang++.o is generated from following source code compiled with IBM xlclang with option -qvisibilty 
+
+; int v = 0;
+;
+; __attribute__((visibility ("protected"))) int  vp = 1;
+; __attribute__((visibility ("hidden"))) int  vh = 2;
+; __attribute__((visibility ("default"))) int  vd = 3;
+; __attribute__ ((weak)) __attribute__((visibility ("hidden"))) int vwh = 4;
+; __attribute__ ((weak)) __attribute__((visibility ("protected"))) int vwp = 5;
+;
+; class C {
+; public:
+;  int c;
+;  C(int v):c(v) {}
+; };
+;
+; C cc(2);
+;
+; static int si = 6;
+;
+; static int func0 () {
+;   return vp+si;
+; }
+;
+; int func1 (int i) {
+;   return func0() * i;
+; }
+;
+; __attribute__ ((weak)) __attribute__((visibility ("hidden")))
+; int fwh() {
+;   return si+1;
+; }
+;
+; __attribute__ ((weak)) __attribute__((visibility ("protected")))
+; int fwp() {
+;   return si+2;
+; }
+
+; test1_gcc.o is generated from following source code with compiled with gcc
+; const char* con= "Test for --symbols";
+; const char myString[]="my string.";
+;  int i;
+;  char c ;
+;  char c1;
+;  char c2;
+;  char *ap;
+;  float f;
+;  long long ll;
+;  static int si;
+;  extern int ei;
+; int  bar(const char *v) {
+;    si = 1;
+;    return (int)v[0] + (int)v[2] + si + ei;
+; }
+;
+; void foo() {
+;    bar(con);
+; }
+;
+; int vd=2;
+
+
+SYM:      _Z3fwpv protected
+SYM-NEXT: _Z5func1i
+SYM-NEXT: _ZN1CC2Ei
+SYM-NEXT: ap
+SYM-NEXT: bar
+SYM-NEXT: c
+SYM-NEXT: c1
+SYM-NEXT: c2
+SYM-NEXT: cc
+SYM-NEXT: con
+SYM-NEXT: f
+SYM-NEXT: foo
+SYM-NEXT: i
+SYM-NEXT: ll
+SYM-NEXT: myString
+SYM-NEXT: v
+SYM-NEXT: vd
+SYM-NEXT: vd export
+SYM-NEXT: vp protected
+SYM-NEXT: vwp protected
+
+

llvm/test/tools/llvm-objdump/XCOFF/export_sym_list_obj.test

+; Test the functionality of the llvm-objdump --export-unique-symbol  exporting  symbol list of xcoff object file. 
+; Test 1: Not export the symbol begin with "__sinit" and "__sterm" and "."
+; Test 2: Not export weak symbol with option "exclude-weak".
+; Test 3: Export format: symbol_name  symbol_visibility(if there is).
+; Test 4: Not export __rsrc symbol name with option "exclude-rsrc".
+; Test 5: Not export symbol which start with "__tf1".
+; Test 6: Not export any symobl for shared object file.
+; Test 7: Only keep unique symbol in the export list.
+; Test 8: export unique symbol for 32 bits and 64 bits xcoff obj file.
+
+; RUN: llvm-objdump --export-unique-symbol %p/Inputs/exp_sym.o | FileCheck --check-prefixes=SYM %s
+; RUN: llvm-objdump --export-unique-symbol %p/Inputs/exp_sym_64.o | FileCheck --check-prefixes=SYM %s
+; RUN: llvm-objdump --export-unique-symbol %p/Inputs/exp_sym.o %p/Inputs/exp_sym.o | FileCheck --check-prefixes=SYM %s
+; RUN: llvm-objdump --export-unique-symbol --exclude-weak %p/Inputs/exp_sym.o | FileCheck --check-prefix=EXCLUDE-WEAK %s
+; RUN: llvm-objdump --export-unique-symbol %p/Inputs/tf-rsrc-gcc.o | FileCheck --check-prefixes=RSRC %s
+; RUN: llvm-objdump --export-unique-symbol --exclude-rsrc %p/Inputs/tf-rsrc-gcc.o  | FileCheck --check-prefixes=EXCLUDE-RSRC --allow-empty %s
+; RUN: llvm-objdump --export-unique-symbol %p/Inputs/libtest_sharedobj.a | FileCheck --check-prefixes=ANY --allow-empty %s
+; RUN: llvm-objdump --export-unique-symbol %p/Inputs/tf-rsrc-gcc.o | FileCheck --check-prefixes=TF1 %s
+
+; The object file Inputs/exp_sym.o is generated from following source code with IBM xlclang with option -qvisibilty 
+ 
+;; int v = 0;
+;; __attribute__((visibility ("protected"))) int  vp = 1;
+;; __attribute__((visibility ("hidden"))) int  vh = 2;
+;; __attribute__((visibility ("default"))) int  vd = 3;
+;; __attribute__ ((weak)) __attribute__((visibility ("hidden"))) int vwh = 4;
+;; __attribute__ ((weak)) __attribute__((visibility ("protected"))) int vwp = 5;
+;; 
+;; class C {
+;; public:
+;;  int c;
+;;  C(int v):c(v) {}
+;; };
+;; 
+;; C cc(2);
+;; 
+;; static int si = 6;
+;; 
+;; static int func0 () {
+;;   return vp+si;
+;; }
+;; 
+;; int func1 (int i) {
+;;   return func0() * i;
+;; }
+;; 
+;; __attribute__ ((weak)) __attribute__((visibility ("hidden")))
+;; int fwh() {
+;;   return si+1;
+;; }
+;; 
+;; __attribute__ ((weak)) __attribute__((visibility ("protected")))
+;; int fwp() {
+;;   return si+2;
+;; }
+
+
+; SYM:      _Z3fwpv protected
+; SYM-NEXT: _Z5func1i
+; SYM-NEXT: _ZN1CC2Ei
+; SYM-NEXT: cc
+; SYM-NEXT: v
+; SYM-NEXT: vd export
+; SYM-NEXT: vp protected
+; SYM-NEXT: vwp protected
+
+; EXCLUDE-WEAK:      _Z5func1i
+; EXCLUDE-WEAK-NEXT: cc
+; EXCLUDE-WEAK-NEXT: v
+; EXCLUDE-WEAK-NEXT: vd export
+; EXCLUDE-WEAK-NEXT: vp protected
+
+; RSRC: __rsrc
+; EXCLUDE-RSRC-NOT: __rsrc
+
+; TF1: __rsrc 
+; TF1-NEXT: lue
+; TF1-NEXT: rc
+; TF1-NEXT: x
+
+; ANY-NOT: .*

llvm/tools/llvm-objdump/ObjdumpOpts.td

def unwind_info : Flag<["--"], "unwind-info">,
   HelpText<"Display unwind information">;
 def : Flag<["-"], "u">, Alias<unwind_info>,
   HelpText<"Alias for --unwind-info">;
 
 def wide : Flag<["--"], "wide">,
   HelpText<"Ignored for compatibility with GNU objdump">;
 def : Flag<["-"], "w">, Alias<wide>;
 
+def export_unique_symbol : Flag<["--"], "export-unique-symbol">,
+  HelpText<"Export symbol list for xcoff object file or archive">;
+
 defm prefix : Eq<"prefix", "Add prefix to absolute paths">,
               MetaVarName<"prefix">;
 defm prefix_strip
     : Eq<"prefix-strip", "Strip out initial directories from absolute "
                          "paths. No effect without --prefix">,
       MetaVarName<"prefix">;
 
 def debug_vars_EQ : Joined<["--"], "debug-vars=">,
def no_symbolic_operands : Flag<["--"], "no-symbolic-operands">,
   Group<grp_mach_o>;
 
 def arch_EQ : Joined<["--"], "arch=">,
   HelpText<"architecture(s) from a Mach-O file to dump">,
   Group<grp_mach_o>;
 def : Separate<["--"], "arch">,
   Alias<arch_EQ>,
   Group<grp_mach_o>;
+
+def grp_xcoff_o : OptionGroup<"kind">, HelpText<"llvm-objdump XCOFF Specific Options">;
+
+def exclude_rsrc : Flag<["--"], "exclude-rsrc">,
+  HelpText<"Exclude the rsrc symbol from export symbol list"
+           "for xcoff (requires --export-unique-symbol)">,
+  Group<grp_xcoff_o>;
+
+def exclude_weak : Flag<["--"], "exclude-weak">,
+  HelpText<"Exclude the weak symbol from export symbol list"
+           "for xcoff files (requires --export-unique-symbol)">,
+  Group<grp_xcoff_o>;
+

llvm/tools/llvm-objdump/XCOFFDump.h

 #define LLVM_TOOLS_LLVM_OBJDUMP_XCOFFDUMP_H
 
 #include "llvm/Object/XCOFFObjectFile.h"
 
 namespace llvm {
 
 struct SymbolInfoTy;
 
+namespace opt {
+class InputArgList;
+} // namespace opt
+
 namespace objdump {
+
 Optional<XCOFF::StorageMappingClass>
 getXCOFFSymbolCsectSMC(const object::XCOFFObjectFile *Obj,
                        const object::SymbolRef &Sym);
 
 Optional<object::SymbolRef>
 getXCOFFSymbolContainingSymbolRef(const object::XCOFFObjectFile *Obj,
                                   const object::SymbolRef &Sym);
 
 bool isLabel(const object::XCOFFObjectFile *Obj, const object::SymbolRef &Sym);
 
 std::string getXCOFFSymbolDescription(const SymbolInfoTy &SymbolInfo,
                                       StringRef SymbolName);
 
 Error getXCOFFRelocationValueString(const object::XCOFFObjectFile *Obj,
                                     const object::RelocationRef &RelRef,
                                     llvm::SmallVectorImpl<char> &Result);
+
+void parseXCOFFOptions(const llvm::opt::InputArgList &InputArgs);
+void exportSymbolInfoFromFile(StringRef InputFile);
+void printExportSymboList(raw_fd_ostream &OS);
+
 } // namespace objdump
 } // namespace llvm
 #endif

llvm/tools/llvm-objdump/XCOFFDump.cpp

 //===-- XCOFFDump.cpp - XCOFF-specific dumper -----------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// This file implements the XCOFF-specific dumper for llvm-objdump.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "XCOFFDump.h"
-
+#include "ObjdumpOptID.h"
 #include "llvm-objdump.h"
 #include "llvm/Demangle/Demangle.h"
+#include "llvm/Option/ArgList.h"
+#include "llvm/Support/Regex.h"
 
 using namespace llvm;
 using namespace llvm::object;
+using namespace llvm::objdump;
+
+static bool ExcludeRsrc;
+static bool ExcludeWeak;
+
+void objdump::parseXCOFFOptions(const llvm::opt::InputArgList &InputArgs) {
+  ExcludeRsrc = InputArgs.hasArg(OBJDUMP_exclude_rsrc);
+  ExcludeWeak = InputArgs.hasArg(OBJDUMP_exclude_weak);
+}
 
 Error objdump::getXCOFFRelocationValueString(const XCOFFObjectFile *Obj,
                                              const RelocationRef &Rel,
                                              SmallVectorImpl<char> &Result) {
   symbol_iterator SymI = Rel.getSymbol();
   if (SymI == Obj->symbol_end())
     return make_error<GenericBinaryError>(
         "invalid symbol reference in relocation entry",
if (SymbolInfo.XCOFFSymInfo.StorageMappingClass &&
       !SymbolInfo.XCOFFSymInfo.IsLabel) {
     const XCOFF::StorageMappingClass Smc =
         SymbolInfo.XCOFFSymInfo.StorageMappingClass.getValue();
     Result.append(("[" + XCOFF::getMappingClassString(Smc) + "]").str());
   }
 
   return Result;
 }
+
+struct SymNameVisibilityTy {
+  std::string SymName;
+  StringRef Visibility;
+  SymNameVisibilityTy(StringRef Name, StringRef Vis)
+      : SymName(Name.str()), Visibility(Vis){};
+  SymNameVisibilityTy(StringRef Name) : SymName(Name.str()){};
+
+private:
+  friend bool operator<(const SymNameVisibilityTy &SNV1,
+                        const SymNameVisibilityTy SNV2) {
+    int NameRes = SNV1.SymName.compare(SNV2.SymName);
+    if (NameRes < 0)
+      return true;
+    if (NameRes > 0)
+      return false;
+
+    return SNV1.Visibility.compare(SNV2.Visibility) == -1;
+  }
+
+  friend bool operator==(const SymNameVisibilityTy &SNV1,
+                         const SymNameVisibilityTy SNV2) {
+    return SNV1.SymName.compare(SNV2.SymName) == 0 &&
+           SNV1.Visibility.compare(SNV2.Visibility) == 0;
+  }
+};
+
+std::vector<SymNameVisibilityTy> ExportSymbols;
+
+static void exportSymbolInfoFromObjectFile(const ObjectFile *O,
+                                           StringRef ObjectName,
+                                           StringRef ArchiveName) {
+
+  if (!O->isXCOFF())
+    return;
+
+  // Skip Shared object file.
+  if (dyn_cast<const XCOFFObjectFile>(O)->getFlags() & XCOFF::F_SHROBJ)
+    return;
+
+  for (const SymbolRef &Sym : O->symbols()) {
+    Expected<StringRef> NameOrErr = Sym.getName();
+    if (!NameOrErr) {
+      reportError(NameOrErr.takeError(), ObjectName, ArchiveName);
+      continue;
+    }
+    StringRef SymName = NameOrErr.get();
+
+    Expected<uint32_t> FlagsOrErr = Sym.getFlags();
+    if (!FlagsOrErr) {
+      reportError(FlagsOrErr.takeError(), ObjectName, ArchiveName);
+    }
+    uint32_t Flags = FlagsOrErr.get();
+    bool Global = Flags & SymbolRef::SF_Global;
+    bool Weak = Flags & SymbolRef::SF_Weak;
+
+    // if the symbol is not ext or weak, not be exported.
+    if (!Global)
+      continue;
+
+    // if the symbol is weak and exclude-weak is enable, not be exported.
+    if (Weak && ExcludeWeak)
+      continue;
+
+    if (Flags & SymbolRef::SF_Hidden || Flags & SymbolRef::SF_Internal)
+      continue;
+
+    Expected<section_iterator> SymSecOrErr = Sym.getSection();
+    if (!SymSecOrErr) {
+      reportError(SymSecOrErr.takeError(), ObjectName, ArchiveName);
+    }
+
+    section_iterator SecIter = SymSecOrErr.get();
+
+    // if the symbol is not text or data section, not be exported.
+    if (SecIter == O->section_end())
+      continue;
+
+    if (!(SecIter->isText() || SecIter->isData() || SecIter->isBSS()))
+      continue;
+
+    Regex r("^__[0-9]+__");
+    if (SymName.startswith("__sinit") || SymName.startswith("__sterm") ||
+        SymName.front() == '.' || SymName.front() == '(' || r.match(SymName))
+      continue;
+
+    if (SymName.startswith("__tf1")) {
+      SymName = SymName.substr(6);
+    } else if (SymName.startswith("__tf9")) {
+      SymName = SymName.substr(14);
+    }
+
+    if (SymName == "__rsrc" && ExcludeRsrc)
+      continue;
+
+    if (Flags & SymbolRef::SF_Exported) {
+      ExportSymbols.push_back(SymNameVisibilityTy(SymName, "export"));
+      continue;
+    }
+
+    if (Flags & SymbolRef::SF_XCOFF_Protected) {
+      ExportSymbols.push_back(SymNameVisibilityTy(SymName, "protected"));
+      continue;
+    }
+
+    ExportSymbols.push_back(SymName);
+  }
+}
+
+/// Export symbols list from  each object file in \a a;
+static void dumpArchive(const Archive *A) {
+  Error Err = Error::success();
+  unsigned I = -1;
+  for (auto &C : A->children(Err)) {
+    ++I;
+    std::string ChildName = getFileNameForError(C, I);
+
+    Expected<std::unique_ptr<Binary>> ChildOrErr = C.getAsBinary();
+    if (!ChildOrErr) {
+      if (auto E = isNotObjectErrorInvalidFileType(ChildOrErr.takeError()))
+        reportError(std::move(E), ChildName, A->getFileName());
+      continue;
+    }
+    if (ObjectFile *O = dyn_cast<ObjectFile>(&*ChildOrErr.get())) {
+      exportSymbolInfoFromObjectFile(O, ChildName, A->getFileName());
+    } else
+      reportError(errorCodeToError(object_error::invalid_file_type), ChildName,
+                  A->getFileName());
+  }
+  if (Err)
+    reportError(std::move(Err), A->getFileName());
+}
+
+void objdump::exportSymbolInfoFromFile(StringRef InputFile) {
+  // Attempt to open the binary.
+  OwningBinary<Binary> OBinary =
+      unwrapOrError(createBinary(InputFile), InputFile);
+  Binary &Binary = *OBinary.getBinary();
+
+  if (Archive *A = dyn_cast<Archive>(&Binary)) {
+    dumpArchive(A);
+  } else if (ObjectFile *O = dyn_cast<ObjectFile>(&Binary))
+    exportSymbolInfoFromObjectFile(O, InputFile, std::string());
+  else
+    reportError(errorCodeToError(object_error::invalid_file_type), InputFile);
+}
+
+void objdump::printExportSymboList(raw_fd_ostream &OS) {
+  if (ExportSymbols.size() == 0)
+    return;
+
+  llvm::sort(ExportSymbols);
+
+  std::vector<SymNameVisibilityTy>::const_iterator Iter = ExportSymbols.begin();
+  std::vector<SymNameVisibilityTy>::const_iterator PreIter = Iter;
+
+  OS << Iter->SymName;
+  if (!Iter->Visibility.empty())
+    OS << " " << Iter->Visibility;
+  OS << "\n";
+
+  PreIter = Iter;
+  while (++Iter < ExportSymbols.end()) {
+    if (!(*PreIter == *Iter)) {
+      OS << Iter->SymName;
+      if (!Iter->Visibility.empty())
+        OS << " " << Iter->Visibility << "\n";
+      else
+        OS << "\n";
+    }
+    PreIter = Iter;
+  }
+}

llvm/tools/llvm-objdump/llvm-objdump.h

 extern bool Relocations;
 extern bool SectionHeaders;
 extern bool SectionContents;
 extern bool ShowRawInsn;
 extern bool SymbolDescription;
 extern bool SymbolTable;
 extern std::string TripleName;
 extern bool UnwindInfo;
+extern bool ExportUniqueSymbol;
 
 extern StringSet<> FoundSectionSet;
 
 typedef std::function<bool(llvm::object::SectionRef const &)> FilterPredicate;
 
 /// A filtered iterator for SectionRefs that skips sections based on some given
 /// predicate.
 class SectionFilterIterator {

llvm/tools/llvm-objdump/llvm-objdump.cpp

 static bool SymbolizeOperands;
 static bool DynamicSymbolTable;
 std::string objdump::TripleName;
 bool objdump::UnwindInfo;
 static bool Wide;
 std::string objdump::Prefix;
 uint32_t objdump::PrefixStrip;
 
+bool objdump::ExportUniqueSymbol;
+
 DebugVarsFormat objdump::DbgVariables = DVDisabled;
 
 int objdump::DbgIndent = 52;
 
 static StringSet<> DisasmSymbolSet;
 StringSet<> objdump::FoundSectionSet;
 static StringRef ToolName;
 
static void dumpObject(ObjectFile *O, const Archive *A = nullptr,
   if (Rebase)
     printRebaseTable(O);
   if (Bind)
     printBindTable(O);
   if (LazyBind)
     printLazyBindTable(O);
   if (WeakBind)
     printWeakBindTable(O);
-
   // Other special sections:
   if (RawClangAST)
     printRawClangAST(O);
   if (FaultMapSection)
     printFaultMaps(O);
 }
 
 static void dumpObject(const COFFImportFile *I, const Archive *A,
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;
+  }
+
   // Attempt to open the binary.
   OwningBinary<Binary> OBinary = unwrapOrError(createBinary(file), file);
   Binary &Binary = *OBinary.getBinary();
 
   if (Archive *A = dyn_cast<Archive>(&Binary))
     dumpArchive(A);
   else if (ObjectFile *O = dyn_cast<ObjectFile>(&Binary))
     dumpObject(O);
static void parseObjdumpOptions(const llvm::opt::InputArgList &InputArgs) {
   parseIntArg(InputArgs, OBJDUMP_stop_address_EQ, StopAddress);
   HasStopAddressFlag = InputArgs.hasArg(OBJDUMP_stop_address_EQ);
   SymbolTable = InputArgs.hasArg(OBJDUMP_syms);
   SymbolizeOperands = InputArgs.hasArg(OBJDUMP_symbolize_operands);
   DynamicSymbolTable = InputArgs.hasArg(OBJDUMP_dynamic_syms);
   TripleName = InputArgs.getLastArgValue(OBJDUMP_triple_EQ).str();
   UnwindInfo = InputArgs.hasArg(OBJDUMP_unwind_info);
   Wide = InputArgs.hasArg(OBJDUMP_wide);
+  ExportUniqueSymbol = InputArgs.hasArg(OBJDUMP_export_unique_symbol);
   Prefix = InputArgs.getLastArgValue(OBJDUMP_prefix).str();
   parseIntArg(InputArgs, OBJDUMP_prefix_strip, PrefixStrip);
   if (const opt::Arg *A = InputArgs.getLastArg(OBJDUMP_debug_vars_EQ)) {
     DbgVariables = StringSwitch<DebugVarsFormat>(A->getValue())
                        .Case("ascii", DVASCII)
                        .Case("unicode", DVUnicode);
   }
   parseIntArg(InputArgs, OBJDUMP_debug_vars_indent_EQ, DbgIndent);
 
   parseMachOOptions(InputArgs);
+  parseXCOFFOptions(InputArgs);
 
   // Parse -M (--disassembler-options) and deprecated
   // --x86-asm-syntax={att,intel}.
   //
   // Note, for x86, the asm dialect (AssemblerDialect) is initialized when the
   // MCAsmInfo is constructed. MCInstPrinter::applyTargetSpecificCLOption is
   // called too late. For now we have to use the internal cl::opt option.
   const char *AsmSyntax = nullptr;
int main(int argc, char **argv) {
   if (DisassembleAll || PrintSource || PrintLines ||
       !DisassembleSymbols.empty())
     Disassemble = true;
 
   if (!ArchiveHeaders && !Disassemble && DwarfDumpType == DIDT_Null &&
       !DynamicRelocations && !FileHeaders && !PrivateHeaders && !RawClangAST &&
       !Relocations && !SectionHeaders && !SectionContents && !SymbolTable &&
       !DynamicSymbolTable && !UnwindInfo && !FaultMapSection &&
+      !ExportUniqueSymbol &&
       !(MachOOpt &&
         (Bind || DataInCode || DylibId || DylibsUsed || ExportsTrie ||
          FirstPrivateHeader || FunctionStarts || IndirectSymbols || InfoPlist ||
          LazyBind || LinkOptHints || ObjcMetaData || Rebase || Rpaths ||
          UniversalHeaders || WeakBind || !FilterSections.empty()))) {
     T->printHelp(ToolName);
     return 2;
   }