[PATCH] D76081: [Object] object::ELFObjectFile::dynamic_symbol_begin(): skip symbol index 0

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D76081#1935530 <https://reviews.llvm.org/D76081#1935530>, @Higuoxing wrote:

> Can we use these tests as `--dynamic` dedicated tests as well (@jhenderson metioned here <https://reviews.llvm.org/D76213#1930891>)?


I think so.



================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:1
+## Test llvm-nm dumping good ELF file with .dynsym section.
+# RUN: yaml2obj --docnum=1 %s -o %t1
----------------
I guess @jhenderson will not be happy to see "good ELF file".
I am not sure it is "good". You should just probably say that
we test how llvm-nm dump dynamic symbols from a valud dynamic section.


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:3
+# RUN: yaml2obj --docnum=1 %s -o %t1
+# RUN: llvm-nm --debug-syms --dynamic %t1 | \
+# RUN: FileCheck %s --match-full-lines --strict-whitespace --check-prefix DYNSYM
----------------
Why do you need `--debug-syms`?


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:11
+# DYNSYM-NEXT:0000000000000000 n localsym2
+# DYNSYM-EMPTY:
+
----------------
We usually format such pieces a bit differently:

```
#       DYNSYM:                 U globalsym
#  DYNSYM-NEXT:                 U localsym1
#  DYNSYM-NEXT:0000000000000000 n localsym2
# DYNSYM-EMPTY:
```


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:30
+  - Name:    localsym2
+    Section: section
+
----------------
I'd probably group locals together.


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:39
+
+# NODYNSYM:{{.*}}: no symbols
+# NODYNSYM-EMPTY:
----------------
Instead of `{{.*}}` you probably want to define and check `FILE`, like we do in many other tests.
E.g.:

```
# RUN: llvm-readobj --dyn-relocations %t1 2>&1 | FileCheck %s -DFILE=%t1 --check-prefix=LLVM
....
# LLVM:      Dynamic Relocations {
# LLVM-NEXT: warning: '[[FILE]]': unable to get name of the dynamic symbol with index 1: st_name (0x1234) is past the end of the string table of size 0x1

```


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:53
+# RUN: FileCheck %s --match-full-lines --strict-whitespace --check-prefix EMPTY
+# RUN: llvm-nm --debug-syms -D %t3 2>&1 | \
+# RUN: FileCheck %s --match-full-lines --strict-whitespace --check-prefix EMPTY
----------------
I think it is enough to verify that `-D` == `--debug-syms` once at the begining. No need to re-test `-D` everywhere.
(See how we test aliases in llvm-readelf, for example).


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:79
+# MALFORMED:{{.*}}: no symbols
+# MALFORMED-EMPTY:
+
----------------
What is the difference with `NODYNSYM` and `EMPTY`? You should be able to use a single check tag.


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:90
+    Type:    SHT_DYNSYM
+    Size:    25 # sizeof(Elf64_Sym) = 24
+    Content: "0123"
----------------
I think you should test 32-bits case too. Looks you can just set `Size` to 1 and adjust this YAML to use `-D BITS=32/64`,
see `llvm-readobj\ELF\hash-histogram.test` for sample.


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:91
+    Size:    25 # sizeof(Elf64_Sym) = 24
+    Content: "0123"
----------------
Doesn't seem you need `Content`? It should be enough either to have `Size` or `Content` I believe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76081