[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 06:28:20 PDT 2020
grimar added inline comments.
================
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)) {
----------------
Higuoxing wrote:
> grimar wrote:
> > grimar wrote:
> > > 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.
> > Let me debug this to see how well my suggestions works. I'll return with something soon.
> What do you think of changing the name to `IsFirstNULLSymbol(SomeSec)` or `IsFirstNULLSymbolFrom(SomeSec)`, and leave other checkers unchanged?
>
> We have to call the function on `.symtab` and `.dynsym` to check whether the symbol is NULL symbol. If we want to add one another function `IsFormatSpecificSymbol()`, we still have to keep `IsFirstNULLSymbol()`
>
> ```
> bool IsFormatSpecificSymbol() {
> if (IsFirstNULLSymbol(.symtab) || IsFirstSymbol(.dynsym))
> return true;
> else if (EF.getHeader()->e_machine == ELF::EM_ARM) {
> ...
> return true;
> } else if (ESym->getType() == STT_FILE || ESym->getType() == STT_SECTION)
> return true;
> return false;
> }
> ```
Right. I've experimented with the code and came to the same conclusion:
```
auto IsNullSymbol = [this](const Elf_Sym *Sym, const Elf_Shdr *Symtab) {
Expected<typename ELFT::SymRange> SymbolsOrErr = EF.symbols(Symtab);
if (!SymbolsOrErr) {
// TODO: Actually report errors helpfully.
consumeError(SymbolsOrErr.takeError());
return false;
}
return Sym == SymbolsOrErr->begin();
};
if (IsNullSymbol(ESym, DotSymtabSec) || IsNullSymbol(ESym, DotDynSymSec))
Result |= SymbolRef::SF_FormatSpecific;
```
Creating a `IsFormatSpecificSymbol` could be a nice follow-up refactoring probably.
So just renaming your version to to `IsNullSymbol` LG to me.
One more thing though: your test check the broken `.symtab` case, but the code also checks the `.dynsym`.
This should also be covered by the test I think.
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