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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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
----------------
MaskRay wrote:
> 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.
One of the problems we have is that we don't know if the use case exists. I'm inclined to agree with you that the combination is realistic though.


================
Comment at: test/tools/llvm-nm/linker-synthesized.test:4
+
+# We used to be wrong with some linker synthesized symbols.
+
----------------
I think it would be better if the comment is at the top of the file.

Up to you, but I've also seen some people recently using '##' to clearly indicate comments versus lit/FileCheck stuff. It might be nice if you add them, but I don't mind if you don't want to.

If you've got a reference to a reported bug, it would be great to add that.


================
Comment at: test/tools/llvm-nm/linker-synthesized.test:6-7
+
+# We mark __init_array_start as 't', as consistent with GNU nm >= 2.33 (older GNU
+# nm marks it as 'd').
+
----------------
This comment doesn't line up with the behaviour we're testing below...


================
Comment at: test/tools/llvm-nm/nobits.test:22
+Symbols:
+  - Name:    mybss_local
+    Binding: STB_LOCAL
----------------
Perhaps for completeness sake, it's worth adding a local TLS symbol too.


================
Comment at: test/tools/llvm-nm/readonly.test:33-35
+  - Name:    global
+    Binding: STB_GLOBAL
+    Section: myrodata0
----------------
Again for completeness, it might be worth adding global versions of all the local symbols. I don't mind too much though.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61551