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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

This need test cases I think.



================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:1024
+      toDRI(DotDynSymSec,
+            DotDynSymSec && DotDynSymSec->sh_size >= sizeof(Elf_Sym) ? 1 : 0);
   return symbol_iterator(SymbolRef(Sym, this));
----------------
grimar wrote:
> Ok. So seems we are going to make this function to ignore the zero symbol.
> 
> Code on the left did not work with `sh_size`, you are adding the logic that uses it,
> but what if `sh_size` is broken? Here, `sh_size  % sizeof(Elf_Sym) != 0`
> means we have a corrupted section isn't?
> 
> I'd suggest to isolate error handling. For now we can probably do something like:
> 
> ```
> // A comment saying we ignore this errors currently.
> if (!DotDynSymSec->sh_size || DotDynSymSec->sh_size % sizeof(Elf_Sym))
>  return dynamic_symbol_end();
> // A comment about what we do.
> return {SymbolRef(toDRI(DotDynSymSec, 1), this};
> ```
> 
> Do we have a test where this method is used when  `sh_size  % sizeof(Elf_Sym) != 0`?
You do not need curly bracers and `else` after `return`. See the sample I've suggested above.

> Do we have a test where this method is used when sh_size  % sizeof(Elf_Sym) != 0?

This question is unanswered.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76081