[PATCH] D76213: [llvm-nm] Add test for `--debug-syms --dynamic`

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D76213#1925340 <https://reviews.llvm.org/D76213#1925340>, @MaskRay wrote:

> `--debug-syms` may be legacy cruft of GNU nm. An x86 nm does not hide `$a` `$t` mapping symbols. I wonder whether we should drop the auto-hide logic.


Mapping symbols are an ARM thing. If we are dumping an ARM object, we should hide the mapping symbols for compatibility (I see people iterating over lists of symbols dumped by nm, so we can't change this behaviour). Obviously, if this isn't an ARM object, they aren't mapping symbols, and should be dumped.

I'm somewhat concerned by the `--implicit-check-not U` usage to ensure no undefined symbol is printed, as it feels fragile. I'd prefer a more stringent check like `--implicit-check-not={{.}}`. That will ensure that there is no output other than the checked output.



================
Comment at: llvm/test/tools/llvm-nm/debug-syms.test:35
+# RUN: yaml2obj --docnum=2 %s -o %t2.o
+# RUN: llvm-nm --debug-syms --dynamic %t2.o | FileCheck %s --implicit-check-not U --check-prefix DYNSYM
+# RUN: llvm-nm -a --dynamic %t2.o | FileCheck %s --implicit-check-not U --check-prefix DYNSYM
----------------
grimar wrote:
> I am not an expert in `llvm-nm`, but is `debug-syms.test` is a right place for this test?
> I'd expect to see a test case for `--dynamic/-D`, but I've not found.
> Seems we want to add one instead?
I would suggest that a --dynamic dedicated test file makes sense, but that this test case here in this file also makes sense.


================
Comment at: llvm/test/tools/llvm-nm/debug-syms.test:5-9
+# SYMBOL:      $a
+# SYMBOL-NEXT: $d
+# SYMBOL-NEXT: $t
+# SYMBOL-NEXT: file_sym
+# SYMBOL-NEXT: section
----------------
Here and below, please add the letter column. This will help ensure the `--implicit-check-not U` is clearer.


================
Comment at: llvm/test/tools/llvm-nm/debug-syms.test:16
+# DYNSYM-NEXT: dynlocal
+
 
----------------
No need for two blank lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76213