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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: test/tools/llvm-nm/X86/data.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
----------------
jhenderson wrote:
> 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?
Created `readonly.test`


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


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


================
Comment at: test/tools/llvm-nm/X86/linker-synthesized.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm -B -S %t | FileCheck %s
----------------
jhenderson wrote:
> What is this test actually testing that is not tested by other tests?
This can be seen as a generalized `init_fini.test`. It is to prevent D26937. I can delete it if you think it not useful.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61551