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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

Almost there.
I also think that comments in the test perhaps need a bit of polishing, but I am not a native speaker to suggest much better versions, so will refrain.



================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:1026
+    return symbol_iterator(SymbolRef(toDRI(DotDynSymSec, 0), this));
+  else
+    // Skip 0-index NULL symbol.
----------------
> You do not need curly bracers and else after return. See the sample I've suggested above

This wasn't addressed. 


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:1
+## Test llvm-nm dumping ELF file with valid .dynsym section.
+# RUN: yaml2obj --docnum=1 %s -o %t1.o
----------------
jhenderson wrote:
> As the switch name is `--dynamic`, let's rename this test to `dynamic.test`.
I'd add a general comment about the whole test at the begining:

"## This is a test for --dynamic/-D option."


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:30
+  - Name:    localsym2
+    Section: section
+
----------------
Normally, local symbols go before globals in a ELF object.
And you do not need to specify "Binding: STB_LOCAL" because `STB_LOCAL` is the default binding for a symbol,
used when "Binding" property is not provided.


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:35
+# RUN: llvm-nm --dynamic %t2.o 2>&1 | \
+# RUN: FileCheck %s --match-full-lines --strict-whitespace -DFILE=%t2.o --check-prefix EMPTY
+
----------------
I'd expect to see `EMPTY` check lines right below this `RUN` lines, not at the end of file.
(that is what we normally do).


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:82
+#       EMPTY:[[FILE]]: no symbols
+# EMPTY-EMPTY:
----------------
See. You have a tag named "EMPTY" now. But it is used to test 3 cases:
1) When there is no `.dynsym` section.
2) When `.dynsym` section is empty.
3) When `.dynsym` section is broken.

`EMPTY` is a valid name only for (2). Hence, should be renamed. Probably `NO-SYMS`?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76081