[llvm-nm] Allow --size-sort to print symbols with only Symbol size

[h4xr]

Summary

When llvm-nm is passed only the --size-sort option for an object file, there is no output generated.
The commit modifies the behavior to print the symbols sorted and their size which is also inline with
the output of the GNU nm tool.

Signed-off-by: Saurabh Badhwar <[e-mail]>

[h4xr]

[PATCH] D56063 Add test for the --size-sort symbol output

[rupprecht]

tools/llvm-nm/llvm-nm.cpp

832

Reverting this part of the change doesn't seem to cause any tests to fail, can you add a test case that covers it?

[h4xr]

Improve test cases for the --size-sort option and check for the output with --print-address enabled

[h4xr]

tools/llvm-nm/llvm-nm.cpp

832

Hi, I have improved the test cases to account for covering the output when PrintAddress is also supplied. Please let me know, if that is the expected test case here, I will be more than happy to improve the test scenario if I understood it wrong :)

[rupprecht]

test/tools/llvm-nm/X86/size-sort.test

1 ↗

(On Diff #179678)

nit: using - instead of _ in check-prefix is more common (i.e. SIZE-SORT-NO-ADDR), ditto for the rest

5 ↗

(On Diff #179678)

Looks like GNU nm will filter out undefined symbols when --size-sort is used. It isn't documented, but seems intentional from the code.
I guess we don't need to emulate that though, just making a note of this.

tools/llvm-nm/llvm-nm.cpp

832

The added test cases are good, but if I change else if(PrintAddress) back to just else, then tests still pass -- meaning there isn't any regression testing for this bit.
Ideally, there should be test coverage that captures what the change here is for, otherwise it will accidentally be reverted at some point.

[h4xr]

Modify tests to make them more inline with the remaining codebase

[h4xr]

test/tools/llvm-nm/X86/size-sort.test

1 ↗

(On Diff #179678)

Have addressed this change in the latest revision

tools/llvm-nm/llvm-nm.cpp

832

So, I just took a look at the code flow once again to see why the tests weren't failing. On a more indepth look, it seems like the printing of the address and size is controlled by the logic starting at line #855 onwards.
As a matter of which, I have reverted the else-if back to the original else statement as it was there before

[rupprecht]

Sorry for the delay. This change looks good as far as I can tell. If not, we'll learn that llvm-nm needs better test coverage :)
Thanks!
Do you need help committing?

[h4xr]

In D56063#1350131, @rupprecht wrote:

Sorry for the delay. This change looks good as far as I can tell. If not, we'll learn that llvm-nm needs better test coverage :)
Thanks!
Do you need help committing?

Hey, yes I might need help with the commit process. Is there some place where I can look at how the commit process works and I can take it from there :)

[rupprecht]

In D56063#1350690, @h4xr wrote:

In D56063#1350131, @rupprecht wrote:

Sorry for the delay. This change looks good as far as I can tell. If not, we'll learn that llvm-nm needs better test coverage :)
Thanks!
Do you need help committing?

Hey, yes I might need help with the commit process. Is there some place where I can look at how the commit process works and I can take it from there :)

Here you go: http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access
Once you have access, steps on using git-svn to commit: http://llvm.org/docs/GettingStarted.html#for-developers-to-work-with-git-svn

tools/llvm-nm/llvm-nm.cpp

if (I->Sym.getRawDataRefImpl().p)
       SymFlags = I->Sym.getFlags();
     else
       SymFlags = I->SymFlags;
 
     bool Undefined = SymFlags & SymbolRef::SF_Undefined;
     bool Global = SymFlags & SymbolRef::SF_Global;
     bool Weak = SymFlags & SymbolRef::SF_Weak;
     if ((!Undefined && UndefinedOnly) || (Undefined && DefinedOnly) ||
-        (!Global && ExternalOnly) || (SizeSort && !PrintAddress) ||
-        (Weak && NoWeakSymbols))
+        (!Global && ExternalOnly) || (Weak && NoWeakSymbols))
       continue;
     if (PrintFileName)
       writeFileName(outs());
     if ((JustSymbolName ||
          (UndefinedOnly && MachO && OutputFormat != darwin)) &&
         OutputFormat != posix) {
       outs() << Name << "\n";
       continue;
for (SymbolListT::iterator I = SymbolList.begin(), E = SymbolList.end();
     }
 
     // Otherwise, print the symbol address and size.
     if (symbolIsDefined(*I)) {
       if (Obj.isIR())
         strcpy(SymbolAddrStr, printDashes);
       else if(MachO && I->TypeChar == 'I')
         strcpy(SymbolAddrStr, printBlanks);
-      else
+      else if(PrintAddress)
         format(printFormat, I->Address)
           .print(SymbolAddrStr, sizeof(SymbolAddrStr));
       format(printFormat, I->Size).print(SymbolSizeStr, sizeof(SymbolSizeStr));
     }
 
     // If OutputFormat is darwin or we are printing Mach-O symbols in hex and
     // we have a MachOObjectFile, call darwinPrintSymbol to print as darwin's
     // nm(1) -m output or hex, else if OutputFormat is darwin or we are