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

759

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

267

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

1676

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

1737

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

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

54

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

724

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

1714

1804–1805

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

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

54

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

54–55

(and reflow as appropriate)

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

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

250

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.

259

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);
}

715

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

716

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

721

Let's make this a member function of NMSymbol.

729

Spell out the type, rather than using auto here.

732

Should this be demangled?

1738–1742

As per MaskRay's comment above.

1802

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

259

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.

715

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.

716

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.

721

thanks

732

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

259

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.

715

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

715

-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

256

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.

267

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

267

716

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

1728–1735

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

1730

Use llvm::isDigit

1755

Consider making this else if, for clarity.

1802–1806

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.

1864

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

256

thanks

267

thanks

267

thanks

1728–1735

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

1728–1735

thanks for simplify

1802–1806

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

1864

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

267

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

1674

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

1802–1806

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.

1864

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?

[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

267

done ,thanks

1674

thanks

1802–1806

thanks

1864

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

[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

719

Nit: clang-format is complaining.

1686

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?

1695–1731

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

1738–1739

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.

1802–1806

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.

1864

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.

[DiggerLin]

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

1686

thanks

1695–1731

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.

1738–1739

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

1695–1731

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

1695–1731

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

1679

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

1679

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

1679

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/include/llvm/BinaryFormat/XCOFF.h

SHR_SYMTAB = 0x8000,  ///< At exec time, create shared symbol table for program
                         ///< (main program only).
   FORK_POLICY = 0x4000, ///< Forktree policy specified (main program only).
   FORK_COR = 0x2000     ///< If _AOUT_FORK_POLICY is set, specify copy-on-reference
                         ///< if this bit is set. Specify copy-on- write otherwise.
                         ///< If _AOUT_FORK_POLICY is 0, this bit is reserved for
                         ///< future use and should be set to 0.
 };
 
+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)

 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_Internal = 1U << 12      // 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

llvm/lib/BinaryFormat/Magic.cpp

file_magic llvm::identify_magic(StringRef Magic) {
   case 'B':
     if (startswith(Magic, "BC\xC0\xDE"))
       return file_magic::bitcode;
     break;
   case '!':
     if (startswith(Magic, "!<arch>\n") || startswith(Magic, "!<thin>\n"))
       return file_magic::archive;
     break;
+  case '<':
+    if (startswith(Magic, "<bigaf>\n"))
+      return file_magic::archive;
+    break;
   case '\177':
     if (startswith(Magic, "\177ELF") && Magic.size() >= 18) {
       bool Data2MSB = Magic[5] == 2;
       unsigned high = Data2MSB ? 16 : 17;
       unsigned low = Data2MSB ? 17 : 16;
       if (Magic[high] == 0) {
         switch (Magic[low]) {
         default:

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;
+
+    // TODO: We do not deal with SYM_V_PROTECTED here.
+
+    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 exporting symbol list from aix archive file with "llvn-nm --export-symbols"
+
+# RUN: llvm-nm --export-symbols --print-file-name %p/Inputs/big_ar_lib.a 2>&1 | FileCheck %s
+
+# CHECK:      llvm-nm{{(\.exe)?}}: warning: {{.*}}big_ar_lib.a(export_invalid.o): for symbol with index 26: csect symbol "invalid_var" with index 26 contains no auxiliary entry
+# CHECK-NEXT:    __rsrc
+# CHECK-NEXT:   export_var export
+# CHECK-NEXT:   protected_var protected
+# CHECK-NEXT:   tf1value
+# CHECK-NEXT:   tf9value
+# CHECK-NEXT:   weak_func

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

+## Test the option "--export-symbols" of llvm-nm: exporting symbol list of xcoff object file. 
+
+## Generate XCOFF object file.
+# RUN: yaml2obj -DFLAG=0x0002 %s -o %t.o
+
+## Generate XCOFF shared object file.
+# RUN: yaml2obj -DFLAG=0x2000 %s -o %t_shared.o
+
+##  Test following symbols:
+##  Not export global symbols begin with "__sinit" , "__sterm" , "." , "("
+##  Not export global symbols begin with regular expression "^__[0-9]+__"
+##  Not export hidden symbols and internal symbols.
+##  Export substring of the global symbol name  begin with "__tf1" and "__tf9"
+##  Export format: symbol_name  symbol_visibility(if there is).
+# RUN: llvm-nm --export-symbols %t.o | FileCheck --check-prefixes=SYM,WEAK-SYM %s
+
+## Test: only export unique symbols.
+# RUN: llvm-nm --export-symbols %t.o %t.o | FileCheck --check-prefixes=SYM,WEAK-SYM %s
+
+## Test: Not export weak symbol with option "exclude-weak".
+# RUN: llvm-nm --export-symbols --no-weak %t.o | FileCheck --check-prefixes=SYM %s
+
+## Test: Not export __rsrc symbol name with option --no-rsrc.
+# RUN: llvm-nm --export-symbols --no-rsrc %t.o  | FileCheck --check-prefixes=NO-RSRC %s
+
+## Test: Not export any symobl for shared object file.
+# RUN: llvm-nm --export-symbols %t_shared.o | FileCheck --check-prefixes=ANY --allow-empty %s
+
+
+--- !XCOFF
+FileHeader:
+  MagicNumber:       0x1DF
+  Flags:             [[FLAG]]
+AuxiliaryHeader:
+  Magic:                 0x10B
+  Version:               0x2
+  TextSectionSize:       0x280
+  DataSectionSize:       0x90
+Sections:
+  - Name:            .text
+    Flags:           [ STYP_TEXT ]
+  - Name:            .data
+    Flags:           [ STYP_DATA ]
+Symbols:
+  - Name:            export_var
+    Section:         .data
+    Type:            0x4000
+    StorageClass:    C_EXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x09
+       StorageMappingClass:    XMC_RW
+  - Name:            __rsrc
+    Section:         .data
+    Type:            0x0
+    StorageClass:    C_EXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x09
+       StorageMappingClass:    XMC_RW
+       SectionOrLength:        0x4
+  - Name:            __sinit
+    Section:         .data
+    Type:            0x0
+    StorageClass:    C_EXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x9
+       StorageMappingClass:    XMC_RW
+       SectionOrLength:        0xC
+  - Name:            __sterm
+    Section:         .data
+    Type:            0x0
+    StorageClass:    C_EXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x09
+       StorageMappingClass:    XMC_RW
+       SectionOrLength:        0xC
+  - Name:            .func
+    Section:         .text
+    Type:            0x20
+    StorageClass:    C_EXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x02
+       StorageMappingClass:    XMC_PR
+  - Name:            (func)
+    Section:         .data
+    Type:            0x0
+    StorageClass:    C_EXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x09
+       StorageMappingClass:    XMC_RW
+       SectionOrLength:        0xC
+  - Name:            __023__
+    Section:         .data
+    Type:            0x0
+    StorageClass:    C_EXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x09
+       StorageMappingClass:    XMC_RW
+       SectionOrLength:        0x4
+  - Name:            __tf1_tf1value
+    Section:         .data
+    Type:            0x00
+    StorageClass:    C_EXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x09
+       StorageMappingClass:    XMC_RW
+  - Name:            __tf9_12345678tf9value
+    Section:         .data
+    Type:            0x0
+    StorageClass:    C_EXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x09
+       StorageMappingClass:    XMC_RW
+  - Name:            weak_func
+    Section:         .data
+    Type:            0x0
+    StorageClass:    C_WEAKEXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x09
+       StorageMappingClass:    XMC_RW
+  - Name:            protected_var
+    Section:         .data
+    Type:            0x3000
+    StorageClass:    C_EXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x02
+       StorageMappingClass:    XMC_RW
+  - Name:            hidden_var
+    Section:         .data
+    Type:            0x2000
+    StorageClass:    C_HIDEXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x09
+       StorageMappingClass:    XMC_RW
+  - Name:            internal_var
+    Section:         .data
+    Type:            0x1000
+    StorageClass:    C_HIDEXT
+    AuxEntries:
+     - Type:                   AUX_CSECT
+       SymbolAlignmentAndType: 0x09
+       StorageMappingClass:    XMC_RW
+
+# SYM:             __rsrc
+# SYM-NEXT:        export_var export
+# SYM-NEXT:        protected_var protected
+# SYM-NEXT:        tf1value
+# SYM-NEXT:        tf9value
+# WEAK-SYM-NEXT:   weak_func
+
+# NO-RSRC:      export_var export
+# NO-RSRC-NEXT: protected_var protected
+# NO-RSRC-NEXT: tf1value
+# NO-RSRC-NEXT: tf9value
+# NO-RSRC-NEXT:   weak_func
+
+# ANY-NOT: .*

llvm/tools/llvm-nm/Opts.td

multiclass Eq<string name, string help> {
   def NAME #_EQ : Joined<["--"], name #"=">, HelpText<help>;
   def : Separate<["--"], name>, Alias<!cast<Joined>(NAME #_EQ)>;
 }
 
 def debug_syms : FF<"debug-syms", "Show all symbols, even debugger only">;
 def defined_only : FF<"defined-only", "Show only defined symbols">;
 defm demangle : BB<"demangle", "Demangle C++ symbol names", "Don't demangle symbol names">;
 def dynamic : FF<"dynamic", "Display dynamic symbols instead of normal symbols">;
 def extern_only : FF<"extern-only", "Show only external symbols">;
 defm format : Eq<"format", "Specify output format: bsd (default), posix, sysv, darwin, just-symbols">, MetaVarName<"<format>">;
 def help : FF<"help", "Display this help">;
 def no_llvm_bc : FF<"no-llvm-bc", "Disable LLVM bitcode reader">;
 def no_sort : FF<"no-sort", "Show symbols in order encountered">;
 def no_weak : FF<"no-weak", "Show only non-weak symbols">;
 def numeric_sort : FF<"numeric-sort", "Sort symbols by address">;
 def print_armap : FF<"print-armap", "Print the archive map">;
 def print_file_name : FF<"print-file-name", "Precede each symbol with the object file it came from">;
 def print_size : FF<"print-size", "Show symbol size as well as address">;
 def quiet : FF<"quiet", "Suppress 'no symbols' diagnostic">;
 defm radix : Eq<"radix", "Radix (o/d/x) for printing symbol Values">, MetaVarName<"<radix>">;
 def reverse_sort : FF<"reverse-sort", "Sort in reverse order">;
 def size_sort : FF<"size-sort", "Sort symbols by size">;
 def special_syms : FF<"special-syms", "Do not filter special symbols from the output">;
 def undefined_only : FF<"undefined-only", "Show only undefined symbols">;
 def version : FF<"version", "Display the version">;
 def without_aliases : FF<"without-aliases", "Exclude aliases from output">, Flags<[HelpHidden]>;
 
 // Mach-O specific options.
 def grp_mach_o : OptionGroup<"kind">, HelpText<"llvm-nm Mach-O Specific Options">;
 
 def add_dyldinfo : FF<"add-dyldinfo", "Add symbols from the dyldinfo not already in the symbol table">, Group<grp_mach_o>;
 def add_inlinedinfo : FF<"add-inlinedinfo", "Add symbols from the inlined libraries, TBD only">, 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>;
 def dyldinfo_only : FF<"dyldinfo-only", "Show only symbols from the dyldinfo">, Group<grp_mach_o>;
 def no_dyldinfo : FF<"no-dyldinfo", "Don't add any symbols from the dyldinfo">, Group<grp_mach_o>;
 def s : F<"s", "Dump only symbols from this segment and section name">, Group<grp_mach_o>;
 def x : F<"x", "Print symbol entry in hex">, Group<grp_mach_o>;
 
+// XCOFF specific options.
+def grp_xcoff_o : OptionGroup<"kind">, HelpText<"llvm-nm XCOFF Specific Options">;
+
+def export_symbols : FF<"export-symbols", "Export symbol list for xcoff object file or archive">, Group<grp_xcoff_o>;
+def no_rsrc : FF<"no-rsrc", "Exclude the rsrc symbol from export symbol list"
+           "for xcoff (requires --export-unique-symbol)">, Group<grp_xcoff_o>;
+
 def : FF<"just-symbol-name", "Alias for --format=just-symbols">, Alias<format_EQ>, AliasArgs<["just-symbols"]>, Flags<[HelpHidden]>;
 def : FF<"portability", "Alias for --format=posix">, Alias<format_EQ>, AliasArgs<["posix"]>;
 
 def : F<"a", "Alias for --debug-syms">, Alias<debug_syms>;
 def : F<"A", "Alias for --print-file-name">, Alias<print_file_name>;
 def : F<"B", "Alias for --format=bsd">, Alias<format_EQ>, AliasArgs<["bsd"]>;
 def : F<"C", "Alias for --demangle">, Alias<demangle>;
 def : F<"D", "Alias for --dynamic">, Alias<dynamic>;

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

 //
 // This "nm" supports many of the features of GNU "nm", including its different
 // output formats.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/BinaryFormat/COFF.h"
+#include "llvm/BinaryFormat/XCOFF.h"
 #include "llvm/Demangle/Demangle.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/Object/Archive.h"
 #include "llvm/Object/COFF.h"
 #include "llvm/Object/COFFImportFile.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Object/IRObjectFile.h"
 #include "llvm/Object/MachO.h"
 #include "llvm/Object/MachOUniversal.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Object/TapiFile.h"
 #include "llvm/Object/TapiUniversal.h"
 #include "llvm/Object/Wasm.h"
 #include "llvm/Object/XCOFFObjectFile.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Program.h"
+#include "llvm/Support/Regex.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/TargetSelect.h"
 #include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <vector>
 
 using namespace llvm;
 using namespace object;

 static bool PrintFileName;
 static bool PrintSize;
 static bool Quiet;
 static bool ReverseSort;
 static bool SpecialSyms;
 static bool SizeSort;
 static bool UndefinedOnly;
 static bool WithoutAliases;
+static bool ExportSymbols; // The option is xcoff specific option.
+static bool NoRsrc;        // The option is xcoff specific option.
 
 namespace {
 enum Radix { d, o, x };
 } // namespace
 static Radix AddressRadix;
 
 // Mach-O specific options.
 static bool ArchAll = false;
 static std::vector<StringRef> ArchFlags;
 static bool AddDyldInfo;
 static bool AddInlinedInfo;
 static bool DyldInfoOnly;
 static bool FormatMachOasHex;
 static bool NoDyldInfo;
 static std::vector<StringRef> SegSect;
 static bool MachOPrintSizeWarning = false;
 
 // Miscellaneous states.
 static bool PrintAddress = true;
 static bool MultipleFiles = false;
 static bool HadError = false;
 
 static StringRef ToolName;
 
-static void warn(Error Err, Twine FileName, Twine Context = Twine()) {
+static void warn(Error Err, Twine FileName, Twine Context = Twine(),
+                 Twine Archive = Twine()) {
   assert(Err);
 
   // Flush the standard output so that the warning isn't interleaved with other
   // output if stdout and stderr are writing to the same place.
   outs().flush();
 
   handleAllErrors(std::move(Err), [&](const ErrorInfoBase &EI) {
     WithColor::warning(errs(), ToolName)
-        << FileName << ": " << (Context.str().empty() ? "" : Context + ": ")
-        << EI.message() << "\n";
+        << (Archive.str().empty() ? FileName : Archive + "(" + FileName + ")")
+        << ": " << (Context.str().empty() ? "" : Context + ": ") << EI.message()
+        << "\n";
   });
 }
 
 static void error(Twine Message, Twine Path = Twine()) {
   HadError = true;
   WithColor::error(errs(), ToolName) << Path << ": " << Message << "\n";
 }
 

 struct NMSymbol {
   uint64_t Address;
   uint64_t Size;
   char TypeChar;
   std::string Name;
   StringRef SectionName;
   StringRef TypeName;
   BasicSymbolRef Sym;
   // The Sym field above points to the native symbol in the object file,
   // for Mach-O when we are creating symbols from the dyld info the above
   // pointer is null as there is no native symbol.  In these cases the fields
   // below are filled in to represent what would have been a Mach-O nlist
   // native symbol.
   uint32_t SymFlags;
   SectionRef Section;
   uint8_t NType;
   uint8_t NSect;
   uint16_t NDesc;
   std::string IndirectName;
+  StringRef Visibility;
 };
 } // anonymous namespace
 
 static bool compareSymbolAddress(const NMSymbol &A, const NMSymbol &B) {
   bool ADefined;
   // Symbol flags have been checked in the caller.
   if (A.Sym.getRawDataRefImpl().p) {
     uint32_t AFlags = cantFail(A.Sym.getFlags());
     ADefined = !(AFlags & SymbolRef::SF_Undefined);
   } else {
     ADefined = A.TypeChar != 'U';
   }
   bool BDefined;
   // Symbol flags have been checked in the caller.
   if (B.Sym.getRawDataRefImpl().p) {
     uint32_t BFlags = cantFail(B.Sym.getFlags());
     BDefined = !(BFlags & SymbolRef::SF_Undefined);
   } else {
     BDefined = B.TypeChar != 'U';
   }
   return std::make_tuple(ADefined, A.Address, A.Name, A.Size) <
          std::make_tuple(BDefined, B.Address, B.Name, B.Size);
 }
 
 static bool compareSymbolSize(const NMSymbol &A, const NMSymbol &B) {
   return std::make_tuple(A.Size, A.Name, A.Address) <
          std::make_tuple(B.Size, B.Name, B.Address);
 }
 
 static bool compareSymbolName(const NMSymbol &A, const NMSymbol &B) {
   return std::make_tuple(A.Name, A.Size, A.Address) <
          std::make_tuple(B.Name, B.Size, B.Address);
 }
 
+static bool compareSymbolNameVisibility(const NMSymbol &A, const NMSymbol &B) {
+  return std::make_tuple(A.Name, A.Visibility) <
+         std::make_tuple(B.Name, B.Visibility);
+}
+
+static bool isSymbolNameVisibilityDuplicate(const NMSymbol &A,
+                                            const NMSymbol &B) {
+  return std::make_tuple(A.Name, A.Visibility) ==
+         std::make_tuple(B.Name, B.Visibility);
+}
+
 static char isSymbolList64Bit(SymbolicFile &Obj) {
   if (auto *IRObj = dyn_cast<IRObjectFile>(&Obj))
     return Triple(IRObj->getTargetTriple()).isArch64Bit();
   if (isa<COFFObjectFile>(Obj) || isa<COFFImportFile>(Obj))
     return false;
   if (XCOFFObjectFile *XCOFFObj = dyn_cast<XCOFFObjectFile>(&Obj))
     return XCOFFObj->is64Bit();
 
if (OutputFormat == posix && !ArchiveName.empty())
     S << ArchiveName << "[" << CurrentFilename << "]: ";
   else {
     if (!ArchiveName.empty())
       S << ArchiveName << ":";
     S << CurrentFilename << ": ";
   }
 }
 
+void printExportSymboList(raw_fd_ostream &OS) {
+  if (SymbolList.size() == 0)
+    return;
+
+  llvm::sort(SymbolList, compareSymbolNameVisibility);
+
+  std::vector<NMSymbol>::const_iterator Iter = SymbolList.begin();
+  std::vector<NMSymbol>::const_iterator PreIter = Iter;
+
+  OS << Iter->Name;
+  if (!Iter->Visibility.empty())
+    OS << " " << Iter->Visibility;
+  OS << "\n";
+
+  PreIter = Iter;
+  while (++Iter < SymbolList.end()) {
+    // ignore duplication symbols which has the same name and visibility.
+    if (!(isSymbolNameVisibilityDuplicate(*PreIter, *Iter))) {
+      OS << Iter->Name;
+      if (!Iter->Visibility.empty())
+        OS << " " << Iter->Visibility << "\n";
+      else
+        OS << "\n";
+    }
+    PreIter = Iter;
+  }
+}
+
 static void sortAndPrintSymbolList(SymbolicFile &Obj, bool printName,
                                    StringRef ArchiveName,
                                    StringRef ArchitectureName) {
   if (!NoSort) {
     using Comparator = bool (*)(const NMSymbol &, const NMSymbol &);
     Comparator Cmp;
     if (NumericSort)
       Cmp = &compareSymbolAddress;
     else if (SizeSort)
       Cmp = &compareSymbolSize;
     else
       Cmp = &compareSymbolName;
 
     if (ReverseSort)
       llvm::sort(SymbolList, [=](const NMSymbol &A, const NMSymbol &B) -> bool {
         return Cmp(B, A);
       });
     else
       llvm::sort(SymbolList, Cmp);
   }
 
   if (!PrintFileName) {
     if ((OutputFormat == bsd || OutputFormat == posix ||
          OutputFormat == just_symbols) &&
         MultipleFiles && printName) {
       outs() << '\n' << CurrentFilename << ":\n";
     } else if (OutputFormat == sysv) {
       outs() << "\n\nSymbols from " << CurrentFilename << ":\n\n";
       if (isSymbolList64Bit(Obj))
         outs() << "Name                  Value           Class        Type"
                << "         Size             Line  Section\n";
       else
         outs() << "Name                  Value   Class        Type"
                << "         Size     Line  Section\n";
     }
   }
 
   const char *printBlanks, *printDashes, *printFormat;
   if (isSymbolList64Bit(Obj)) {
     printBlanks = "                ";
     printDashes = "----------------";
     switch (AddressRadix) {
     case Radix::o:
       printFormat = OutputFormat == posix ? "%" PRIo64 : "%016" PRIo64;
       break;
     case Radix::x:
       printFormat = OutputFormat == posix ? "%" PRIx64 : "%016" PRIx64;
       break;
     default:
       printFormat = OutputFormat == posix ? "%" PRId64 : "%016" PRId64;
     }
   } else {
     printBlanks = "        ";
     printDashes = "--------";
     switch (AddressRadix) {
     case Radix::o:
       printFormat = OutputFormat == posix ? "%" PRIo64 : "%08" PRIo64;
       break;
     case Radix::x:
       printFormat = OutputFormat == posix ? "%" PRIx64 : "%08" PRIx64;
       break;
     default:
       printFormat = OutputFormat == posix ? "%" PRId64 : "%08" PRId64;
     }
   }
 
   for (const NMSymbol &S : SymbolList) {
     uint32_t SymFlags;
     std::string Name = S.Name;
     MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
     if (Demangle) {
       if (Optional<std::string> Opt = demangle(S.Name, MachO))
         Name = *Opt;
if (FunctionStartsAdded) {
           Q += strlen(Q) + 1;
         }
         I++;
       }
     }
   }
 }
 
+static void exportSymbolsForXCOFF(XCOFFObjectFile *Obj, StringRef ArchiveName) {
+
+  // Skip Shared object file.
+  if (Obj->getFlags() & XCOFF::F_SHROBJ)
+    return;
+
+  for (const SymbolRef &Sym : Obj->symbols()) {
+    Expected<StringRef> NameOrErr = Sym.getName();
+
+    if (!NameOrErr) {
+      warn(NameOrErr.takeError(), Obj->getFileName(),
+           "for symbol with index " +
+               Twine(Obj->getSymbolIndex(Sym.getRawDataRefImpl().p)),
+           ArchiveName);
+      return;
+    }
+
+    Expected<uint32_t> SymFlagsOrErr = Sym.getFlags();
+    if (!SymFlagsOrErr) {
+      warn(SymFlagsOrErr.takeError(), Obj->getFileName(),
+           "for symbol with index " +
+               Twine(Obj->getSymbolIndex(Sym.getRawDataRefImpl().p)),
+           ArchiveName);
+      return;
+    }
+
+    uint32_t Flags = *SymFlagsOrErr;
+    StringRef SymName = *NameOrErr;
+    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 option --no_weak is enable, not be exported.
+    if (Weak && NoWeakSymbols)
+      continue;
+
+    if (Flags & SymbolRef::SF_Hidden || Flags & SymbolRef::SF_Internal)
+      continue;
+
+    Expected<section_iterator> SymSecOrErr = Sym.getSection();
+    if (!SymSecOrErr) {
+      warn(SymSecOrErr.takeError(), Obj->getFileName(),
+           "for symbol with index " +
+               Twine(Obj->getSymbolIndex(Sym.getRawDataRefImpl().p)),
+           ArchiveName);
+      continue;
+    }
+
+    section_iterator SecIter = *SymSecOrErr;
+
+    // if the symbol is not text or data section, not be exported.
+    if (SecIter == Obj->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" && NoRsrc)
+      continue;
+
+    NMSymbol S = {};
+    S.Name = SymName.str();
+
+    XCOFFObjectFile *XCOFFObj = dyn_cast<XCOFFObjectFile>(Obj);
+    assert(XCOFFObj && "Not a XCOFFObjectFile in exportSymbolsForXCOFF.");
+    // There is no visibility in old 32 bit XCOFF object file interpret.
+    if (XCOFFObj->is64Bit() || (XCOFFObj->auxiliaryHeader32() &&
+                                (XCOFFObj->auxiliaryHeader32()->getVersion() ==
+                                 XCOFF::NEW_XCOFF_INTERPRET))) {
+      XCOFFSymbolRef XCOFFSym = XCOFFObj->toSymbolRef(Sym.getRawDataRefImpl());
+      uint16_t SymType = XCOFFSym.getSymbolType();
+      if ((SymType & XCOFF::VISIBILITY_MASK) == XCOFF::SYM_V_PROTECTED)
+        S.Visibility = "protected";
+
+      if ((SymType & XCOFF::VISIBILITY_MASK) == XCOFF::SYM_V_EXPORTED)
+        S.Visibility = "export";
+    }
+    SymbolList.push_back(S);
+  }
+}
+
 static void dumpSymbolNamesFromObject(SymbolicFile &Obj, bool printName,
                                       StringRef ArchiveName = {},
                                       StringRef ArchitectureName = {}) {
   auto Symbols = Obj.symbols();
   std::vector<VersionEntry> SymbolVersions;
   if (DynamicSyms) {
     const auto *E = dyn_cast<ELFObjectFileBase>(&Obj);
     if (!E) {
static void dumpSymbolNamesFromObject(SymbolicFile &Obj, bool printName,
   MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
   if (!SegSect.empty() && MachO) {
     Nsect = getNsectForSegSect(MachO);
     // If this section is not in the object file no symbols are printed.
     if (Nsect == 0)
       return;
   }
   if (!(MachO && DyldInfoOnly)) {
+
+    if (ExportSymbols && Obj.isXCOFF()) {
+      XCOFFObjectFile *XCOFFObj = dyn_cast<XCOFFObjectFile>(&Obj);
+      assert(XCOFFObj && "Not a XCOFFObjectFile in exportSymbolsForXCOFF.");
+      exportSymbolsForXCOFF(XCOFFObj, ArchiveName);
+      return;
+    }
+
     size_t I = -1;
     for (BasicSymbolRef Sym : Symbols) {
       ++I;
       Expected<uint32_t> SymFlagsOrErr = Sym.getFlags();
       if (!SymFlagsOrErr) {
         error(SymFlagsOrErr.takeError(), Obj.getFileName());
         return;
       }
for (BasicSymbolRef Sym : Symbols) {
         } else
           error(std::move(E), Obj.getFileName());
       }
       if (!SymbolVersions.empty() && !SymbolVersions[I].Name.empty())
         S.Name +=
             (SymbolVersions[I].IsVerDef ? "@@" : "@") + SymbolVersions[I].Name;
 
       S.Sym = Sym;
       SymbolList.push_back(S);
     }
   }
 
   // If this is a Mach-O file where the nlist symbol table is out of sync
   // with the dyld export trie then look through exports and fake up symbols
   // for the ones that are missing (also done with the -add-dyldinfo flag).
   // This is needed if strip(1) -T is run on a binary containing swift
   // language symbols for example.  The option -only-dyldinfo will fake up
   // all symbols from the dyld export trie as well as the bind info.
   if (MachO && !NoDyldInfo)
     dumpSymbolsFromDLInfoMachO(*MachO);
 
   CurrentFilename = Obj.getFileName();
 
   if (Symbols.empty() && SymbolList.empty() && !Quiet) {
     writeFileName(errs(), ArchiveName, ArchitectureName);
     errs() << "no symbols\n";
   }
 
   sortAndPrintSymbolList(Obj, printName, ArchiveName, ArchitectureName);
 }
 
 // checkMachOAndArchFlags() checks to see if the SymbolicFile is a Mach-O file
 // and if it is and there is a list of architecture flags is specified then
 // check to make sure this Mach-O file is one of those architectures or all
 // architectures was specificed.  If not then an error is generated and this
 // routine returns false.  Else it returns true.
 static bool checkMachOAndArchFlags(SymbolicFile *O, std::string &Filename) {
int main(int argc, char **argv) {
 
   // Mach-O specific options.
   FormatMachOasHex = Args.hasArg(OPT_x);
   AddDyldInfo = Args.hasArg(OPT_add_dyldinfo);
   AddInlinedInfo = Args.hasArg(OPT_add_inlinedinfo);
   DyldInfoOnly = Args.hasArg(OPT_dyldinfo_only);
   NoDyldInfo = Args.hasArg(OPT_no_dyldinfo);
 
+  // XCOFF specific options.
+  ExportSymbols = Args.hasArg(OPT_export_symbols);
+  NoRsrc = Args.hasArg(OPT_no_rsrc);
+
   // llvm-nm only reads binary files.
   if (error(sys::ChangeStdinToBinary()))
     return 1;
 
   // These calls are needed so that we can read bitcode correctly.
   llvm::InitializeAllTargetInfos();
   llvm::InitializeAllTargetMCs();
   llvm::InitializeAllAsmParsers();
int main(int argc, char **argv) {
   if (InputFilenames.size() > 1)
     MultipleFiles = true;
 
   if (NoDyldInfo && (AddDyldInfo || DyldInfoOnly))
     error("--no-dyldinfo can't be used with --add-dyldinfo or --dyldinfo-only");
 
   llvm::for_each(InputFilenames, dumpSymbolNamesFromFile);
 
+  if (ExportSymbols)
+    printExportSymboList(outs());
+
   if (HadError)
     return 1;
 }