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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: test/tools/llvm-nm/bss.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
----------------
jhenderson wrote:
> 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.
Good point to rename it to `nobits.test`, but I don't think it should test `SHF_ALLOC` or `SHF_EXECINSTR`. Their combination with `SHT_NOBITS` is unrealistic. We can check the precedence here, but I think we should only do it when a use case rises.


================
Comment at: test/tools/llvm-nm/data.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
----------------
jhenderson wrote:
> 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.
I've thought it for a while. Since we use the real names `data.test` `readonly.test` etc for the tests, if the symbols/sections in the tests are realistic seem less important. So I'll go with your suggestion.


================
Comment at: test/tools/llvm-nm/nonalloc.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-nm --no-sort %t | FileCheck %s
----------------
jhenderson wrote:
> 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?
That will actually belong to my next patch :) I'll try coming up with some tests though I can't think of realistic use cases of symbols pointing to non-allocated non-debug sections.



================
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)
----------------
jhenderson wrote:
> 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.
I agree but I'm not sure whether we should check the precedence. SHF_EXECINST+SHT_NOBITS doesn't seem realistic.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61551