[PATCH] D98803: [llvm-symbolizer][llvm-nm] Fix AArch64 and ARM mapping symbols handling.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/DebugInfo/Symbolize/ELF/aarch64-mapping-symbol.s:3
+## Ignore AArch64 mapping symbols (with a prefix of $d or $x).
+# RUN: llvm-mc -filetype=obj -triple=aarch64-- %s -o %t
+# RUN: llvm-nm --special-syms %t | FileCheck %s -check-prefix MAPPING_SYM
----------------
I have this vague, and quite possibly incorrect, idea that you can just omit the "--" part of the triple completely (i.e. `-triple=aarch64`).


================
Comment at: llvm/test/DebugInfo/Symbolize/ELF/aarch64-mapping-symbol.s:4
+# RUN: llvm-mc -filetype=obj -triple=aarch64-- %s -o %t
+# RUN: llvm-nm --special-syms %t | FileCheck %s -check-prefix MAPPING_SYM
+
----------------
Add a comment explaining why the llvm-nm line is useful. Ditto for the ARM case.


================
Comment at: llvm/test/tools/llvm-nm/ARM/special-syms.test:2
+## Test --special-syms flag for ARM mapping symbols used to mark transitions
+## between ARM code, THUMB code and data ($a, $t, $t)
+#
----------------



================
Comment at: llvm/test/tools/llvm-nm/debug-syms.test:2
 # RUN: yaml2obj %s -o %t.o
-# RUN: llvm-nm --special-syms --debug-syms %t.o | FileCheck %s --implicit-check-not {{.}} --check-prefix SYMBOL
-# RUN: llvm-nm --special-syms -a %t.o | FileCheck %s --implicit-check-not {{.}} --check-prefix SYMBOL
+# RUN: llvm-nm --debug-syms %t.o | FileCheck %s --implicit-check-not {{.}} --check-prefix SYMBOL
+# RUN: llvm-nm -a %t.o | FileCheck %s --implicit-check-not {{.}} --check-prefix SYMBOL
----------------
Does this behaviour (that mapping symbols are printed with --debug-syms and not --special-syms) match GNU nm behaviour?


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:1808
+      // Don't drop format specifc symbols for ARM and AArch64 ELF targets, they
+      // are used to repesent mapping symbols and needed to honor --special-syms
+      // option.
----------------
jhenderson wrote:
> Also, clang-format the new code below.
Don't forget to run clang-format on this. I think there's a trailing space on the second line of the comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98803/new/

https://reviews.llvm.org/D98803