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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:1024
+      DotDynSymSec->sh_size % sizeof(Elf_Sym) != 0)
+    // We are ignoring errors that sh_size doesn't match symbol entries.
+    return symbol_iterator(SymbolRef(toDRI(DotDynSymSec, 0), this));
----------------
I'd suggest the following comment instead:

"Ignore errors here where the dynsym is empty or does not have a size that is a multiple of the symbol size. These should be handled elsewhere."


================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:1034
   const Elf_Shdr *SymTab = DotDynSymSec;
-  if (!SymTab)
+  if (!SymTab || SymTab->sh_size % sizeof(Elf_Sym) != 0)
     return dynamic_symbol_begin();
----------------
This should have a comment. Perhaps "Do not allow iteration over a dynamic symbol table with a malformed size."


================
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
----------------
As the switch name is `--dynamic`, let's rename this test to `dynamic.test`.


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:4
+# RUN: llvm-nm --dynamic %t1.o | \
+# RUN: FileCheck %s --match-full-lines --strict-whitespace --check-prefix DYNSYM
+# RUN: llvm-nm -D %t1.o | \
----------------
Indent continuation lines like this slightly to make it clear that they are continuations. I find it helps readability of the test:
```
# RUN: llvm-nm --dynamic %t1.o | \
# RUN:   FileCheck %s --match-full-lines --strict-whitespace --check-prefix DYNSYM
```

Same goes throughout this test.


================
Comment at: llvm/test/tools/llvm-nm/dynamic-symbols.test:60-61
+
+## Test llvm-nm dumping ELF file with malformed .dynsym section header
+## whose sh_size doesn't match dynamic symbol entries (sh_size % sizeof(Elf_Sym) != 0).
+# RUN: yaml2obj --docnum=4 -DBITS=32 %s -o %t4-32.o
----------------
What does GNU nm do in this situation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76081