Page MenuHomePhabricator

llvm-nm: Implement --special-syms.
ClosedPublic

Authored by pcc on Jun 19 2020, 7:07 PM.

Diff Detail

Event Timeline

pcc created this revision.Jun 19 2020, 7:07 PM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay accepted this revision.Jun 19 2020, 9:38 PM

LGTM, with a nit about tests. Please give some time for other reviewers to respond.

llvm/test/tools/llvm-nm/AArch64/special-syms.test
2–3

Consider using ## for comments and moving to the top.

(Most newer binary utility tests use ## and some reviewers find the marker makes comments stand out.)

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

In BFD the logic is more complex:

  • bfd/cpu-arm.c:bfd_is_arm_special_symbol_name
  • bfd/cpu-aarch64.c:bfd_is_aarch64_special_symbol_name

I think the simplified rule should be fine.

This revision is now accepted and ready to land.Jun 19 2020, 9:38 PM
jhenderson added a comment.EditedJun 22 2020, 12:43 AM

Seems reasonable, but the llvm-nm documentation needs updating (see llvm/docs/CommandGuide/llvm-nm.rst).

It may also be worth release-noting the change in behaviour too, since it will mean current ARM users who use llvm-nm will see their mapping symbols disappear from the output, if they aren't already using --special-syms.

pcc marked an inline comment as done.Jun 22 2020, 1:07 PM

Seems reasonable, but the llvm-nm documentation needs updating (see llvm/docs/CommandGuide/llvm-nm.rst).

It may also be worth release-noting the change in behaviour too, since it will mean current ARM users who use llvm-nm will see their mapping symbols disappear from the output, if they aren't already using --special-syms.

Done.

This revision was automatically updated to reflect the committed changes.