This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by h4xr on Dec 24 2018, 3:57 AM.

Details

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 <sbsaurabhbadhwar9@gmail.com>

Diff Detail

Event Timeline

h4xr created this revision.Dec 24 2018, 3:57 AM
h4xr updated this revision to Diff 179467.Dec 24 2018, 5:36 AM

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

rupprecht added inline comments.
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?

rupprecht removed a subscriber: rupprecht.
h4xr updated this revision to Diff 179678.Dec 28 2018, 11:36 PM

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

h4xr marked an inline comment as done.Dec 28 2018, 11:40 PM
h4xr added inline comments.
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 marked an inline comment as done.Jan 2 2019, 2:28 PM
rupprecht added inline comments.
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 updated this revision to Diff 180072.Jan 3 2019, 7:16 AM

Modify tests to make them more inline with the remaining codebase

h4xr marked 4 inline comments as done and an inline comment as not done.Jan 3 2019, 7:20 AM
h4xr added inline comments.
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

h4xr marked 2 inline comments as done and an inline comment as not done.Jan 3 2019, 7:21 AM
rupprecht accepted this revision.Jan 8 2019, 12:56 PM

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?

This revision is now accepted and ready to land.Jan 8 2019, 12:56 PM
h4xr added a comment.Jan 8 2019, 10:23 PM

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

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

This revision was automatically updated to reflect the committed changes.