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

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.

Requesting changes due to the difference versus GNU.



================
Comment at: test/tools/llvm-nm/bss.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
----------------
This test should probably be renamed to nobits.test, since that's what is important. It might also be worth showing the behaviour with and without SHF_ALLOC and SHF_EXECINSTR, to show that the section type has precedence here.


================
Comment at: test/tools/llvm-nm/data.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
----------------
I'd still like the names of symbols and sections in this test to be more clearly arbitrary, since the code doesn't care what the names are (but a naive user might think that it does). For example, perhaps you could name your sections simply .foo, .bar, etc or .nobits_tls_alloc_write or something, depending on whether you think the name should reflect the flags or not.

The same goes for names in the other tests.


================
Comment at: test/tools/llvm-nm/data.test:20
+    Type: SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_WRITE ]
+  - Name: .tbss
----------------
These should be SHF_TLS, right? Otherwise, what's the point in testing them in addition to .data?


================
Comment at: test/tools/llvm-nm/data.test:28-29
+Symbols:
+  - Name:    rodata_local
+    Section: .rodata
+  - Name:    tbss_local
----------------
Delete this symbol.


================
Comment at: test/tools/llvm-nm/data.test:37-39
+  - Name:    rodata
+    Binding: STB_GLOBAL
+    Section: .rodata
----------------
Delete this symbol.


================
Comment at: test/tools/llvm-nm/linker-synthesized.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm -B -S %t | FileCheck %s
----------------
In particular in this test, it would be helpful to have a comment explaining why this test is needed. It may also be useful to have similar comments in the others, although those are clearer.


================
Comment at: test/tools/llvm-nm/linker-synthesized.test:3
+# RUN: llvm-nm -B -S %t | FileCheck %s
+!ELF
+FileHeader:
----------------
Couple of nits here:
1) Blank line between the RUN and yaml blocks, for readability.
2) You might want to consider moving the checks before the yaml to be consistent with the other tests you wrote.


================
Comment at: test/tools/llvm-nm/nonalloc.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
----------------
Up to you whether you think it is, but perhaps it would be good to test other debug sections with different flags, e.g. SHF_WRITE?


================
Comment at: test/tools/llvm-nm/readonly.test:14-16
+  - Name: .rodata
+    Type: SHT_PROGBITS
+    Flags: [ SHF_ALLOC, SHF_MERGE, SHF_STRINGS ]
----------------
I'd be inclined to have a couple of different rodata sections, with different combinations of flags (i.e. one with just SHF_ALLOC, another with SHF_MERGE and SHF_ALLOC etc).


================
Comment at: tools/llvm-nm/llvm-nm.cpp:908-909
       return 'b';
-    case ELF::SHT_INIT_ARRAY:
-    case ELF::SHT_FINI_ARRAY:
+    if (Flags & ELF::SHF_EXECINSTR)
       return 't';
+    if (Flags & ELF::SHF_ALLOC)
----------------
After some experimentation, I discovered that GNU nm (at least version 2.24) has SHF_EXECINSTR as higher priority than the SHT_NOBITS section type, so this should be above the previous if.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61551