[PATCH] D77289: [Object] Fix crash caused by unhandled error.
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 2 05:23:36 PDT 2020
grimar added inline comments.
================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:241
using Elf_Dyn = typename ELFT::Dyn;
+ using Elf_Sym_Range = typename ELFT::SymRange;
----------------
You probably do not need it anymore as it is used in the single place now.
================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:629
- auto DotSymtabSecSyms = EF.symbols(DotSymtabSec);
- if (DotSymtabSecSyms && ESym == (*DotSymtabSecSyms).begin())
- Result |= SymbolRef::SF_FormatSpecific;
- auto DotDynSymSecSyms = EF.symbols(DotDynSymSec);
- if (DotDynSymSecSyms && ESym == (*DotDynSymSecSyms).begin())
+ auto CheckSymbol = [&](const Elf_Shdr *Sec) {
+ if (Expected<Elf_Sym_Range> SymbolsOrErr = EF.symbols(Sec)) {
----------------
With this implementation the `CheckSymbol` name probably does not make much sense anymore?
It is unclear what does it check I think. It perhaps should be changed to `IsFormatSpecificSymbol` or something better.
In this case I'd expect to see a bit more related code inside.
E.g. seems the following piece from above:
```
if (ESym->getType() == ELF::STT_FILE || ESym->getType() == ELF::STT_SECTION)
Result |= SymbolRef::SF_FormatSpecific;
```
and the code for ARM from below :
```
if (EF.getHeader()->e_machine == ELF::EM_ARM) {
if (Expected<StringRef> NameOrErr = getSymbolName(Sym)) {
StringRef Name = *NameOrErr;
if (Name.startswith("$d") || Name.startswith("$t") ||
Name.startswith("$a"))
Result |= SymbolRef::SF_FormatSpecific;
```
can live there so that `IsFormatSpecificSymbol` would return `true` for all cases when we want to set the `SF_FormatSpecific` flag.
================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:631
+ if (Expected<Elf_Sym_Range> SymbolsOrErr = EF.symbols(Sec)) {
+ if (ESym == SymbolsOrErr->begin())
+ return true;
----------------
This condition needs a comment to explain what it does.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77289/new/
https://reviews.llvm.org/D77289
More information about the llvm-commits
mailing list