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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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
----------------
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?


================
Comment at: llvm/test/tools/llvm-nm/debug-syms.test:36
+# 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
+
----------------
You have already tested --debug-syms/-a at the first lines. No need to test them again.
Here I'd expect to see `--dynamic/-D`.


================
Comment at: llvm/test/tools/llvm-nm/debug-syms.test:55
+    Type:    STT_OBJECT
+    Binding: STB_LOCAL
+    Section: .data
----------------
Usually local symbols go first.
Also, `STB_LOCAL` is the default binding, so you do not need to set it for locals.


================
Comment at: llvm/test/tools/llvm-nm/debug-syms.test:56
+    Binding: STB_LOCAL
+    Section: .data
+
----------------
Is it important to have "Section"?




================
Comment at: llvm/test/tools/llvm-nm/debug-syms.test:59
+# DYNSYM:      dynglobal
+# DYNSYM-NEXT: dynlocal
----------------
I think you can add `DynamicSymbols` to the first description and have a single YAML.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76213