[PATCH] D77289: [Object] Fix crash caused by unhandled error.

[Xing GUO via Phabricator via llvm-commits]

Higuoxing marked an inline comment as done.
Higuoxing 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)) {
----------------
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;
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77289