This is an archive of the discontinued LLVM Phabricator instance.

[llvm-nm][test] Add additional test coverage for llvm-nm options
ClosedPublic

Authored by jhenderson on Feb 12 2021, 6:27 AM.

Details

Summary

Some of these options have a degree of incidental coverage, or are for Mach-O only. This patch adds dedicated ELF (where applicable) coverage.

Depends on D96601.

Diff Detail

Event Timeline

jhenderson requested review of this revision.Feb 12 2021, 6:27 AM
jhenderson created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2021, 6:27 AM
jhenderson edited the summary of this revision. (Show Details)Feb 12 2021, 6:27 AM
Higuoxing accepted this revision.Feb 12 2021, 7:23 AM

I only have one inline comment. Otherwise LGTM :)

llvm/test/tools/llvm-nm/reverse-sort.test
4–5

I think it might be good to assign different values to symbol size and value. e.g.,

- Name:    second
  Section: .text
  Value:   2
  Size:    3
- Name:    third
  Section: .text
  Value:   3
  Size:    1
- Name:    first
  Section: .text
  Value:   1
  Size:    2

to show that symbols are sorted by their address or size when --numeric-sort or --size-sort option is applied.

This revision is now accepted and ready to land.Feb 12 2021, 7:23 AM
rupprecht accepted this revision.Feb 12 2021, 2:18 PM
jhenderson added inline comments.Feb 15 2021, 1:51 AM
llvm/test/tools/llvm-nm/reverse-sort.test
4–5

It was a deliberate choice to keep the order the same regardless of sorting mode, as there are other tests that show that numeric-sort/size-sort/default sort all work as expected. Changing them so that the sorting order is different for each mode would complicate the test.

The current implementation is pretty clear, in that the reversing is done on the sorted array of symbols. I think for this to change, without breaking at least some part of this test in its current form would be quite difficult.

What do you think?

Higuoxing added inline comments.Feb 15 2021, 2:13 AM
llvm/test/tools/llvm-nm/reverse-sort.test
4–5

Thanks for the explanation. I think you're right.

This revision was landed with ongoing or failed builds.Feb 15 2021, 5:57 AM
This revision was automatically updated to reflect the committed changes.