[PATCH] D61551: [llvm-nm] Fix handling of symbol types 't' 'd' 'r'

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: chrisjackson.
jhenderson added a comment.

Since you're using yaml2obj, can these tests be pulled up out of the X86 directory, into the top-level llvm-nm test directory? That would allow for testing even if X86 is unsupported.



================
Comment at: test/tools/llvm-nm/X86/bss.test:9-12
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
----------------
Nit: not that it really matters, but you could reduce the amount of padding here a bit. Similar comments apply to the other tests.


================
Comment at: test/tools/llvm-nm/X86/data.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
----------------
This test seems needlessly complicated for what it's testing (I think). I think it would also be nice to name the symbols and sections according to what is actually important for this test case (i.e. it's not the name ".data" that is important, it's the section flags/type etc).

How about splitting off the read-only (r/R) symbols into a different test case?


================
Comment at: test/tools/llvm-nm/X86/data.test:38-39
+Symbols:
+  - Name:    __FRAME_END__
+    Section: .eh_frame
+  - Name:    rodata_local
----------------
Do you really need this symbol and the .eh_frame section for this test? What is it showing different to the regular data symbols?


================
Comment at: test/tools/llvm-nm/X86/data.test:42-45
+  - Name:    tbss_local
+    Section: .tdata
+  - Name:    tdata_local
+    Section: .tdata
----------------
These symbols should be STT_TLS or not exist at all, as otherwise they aren't showing us anything interesting.


================
Comment at: test/tools/llvm-nm/X86/linker-synthesized.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm -B -S %t | FileCheck %s
----------------
What is this test actually testing that is not tested by other tests?


================
Comment at: test/tools/llvm-nm/X86/nonalloc.test:3
+# RUN: llvm-nm --no-sort %t | FileCheck %s
+!ELF
+FileHeader:
----------------
Nit: new line before the yaml blob for readability.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61551