[PATCH] D77864: [llvm-nm/objdump/size] Enable error reporting in clients when getSymbolFlags() fails.

[Xing GUO via Phabricator via llvm-commits]

Higuoxing marked an inline comment as done.
Higuoxing added inline comments.


================
Comment at: llvm/test/tools/llvm-nm/invalid-symbol-table-size.test:12
+
+# CHECK: error: {{.*}} section [index 2] has an invalid sh_size ([[SIZE]]) which is not a multiple of its sh_entsize ([[SYMSIZE]])
+
----------------
grimar wrote:
> grimar wrote:
> > Higuoxing wrote:
> > > grimar wrote:
> > > > In this and other test cases you have 2 sections: `.symtab` and `.dynsym`, but only one section (with index 2)
> > > > is reported in error messages.
> > > Yes, because there are malformed `.symtab` and `.dynsym` symbol tables in one ELF file. And `getSymbolFlags()` checks `.symtab` first. See [ELFObjectFile.h](https://github.com/llvm/llvm-project/blob/edcfc391e142ea4f2c5d9dc4976644dd2cecdcae/llvm/include/llvm/Object/ELFObjectFile.h#L629).
> > > 
> > > ```
> > >   if (Expected<typename ELFT::SymRange> SymbolsOrErr =
> > >           EF.symbols(DotSymtabSec)) {
> > >     ...
> > >   } else
> > >     // TODO: Test this error.
> > >     return SymbolsOrErr.takeError();
> > > 
> > >   if (Expected<typename ELFT::SymRange> SymbolsOrErr =
> > >           EF.symbols(DotDynSymSec)) {
> > >     ...
> > >   } else
> > >     // TODO: Test this error.
> > >     return SymbolsOrErr.takeError();
> > > ```
> > > 
> > > I think it's a good point, and it's helpful to report errors on our interested sections.
> > > 
> > > ----------------------------
> > > 
> > > Currently, I have two approaches in mind.
> > > 
> > > 1) We could add one more argument `IsDynamic` to `getSymbolFlags(DataRefImpl Symb, bool IsDynamic=false)`. So that we can skip first checker if we've already known the symbol is a dynamic symbol. (Possible)
> > > 
> > > 2) Change the return type of symbol iterator API from `symbol_iterator` to `Expected<symbol_iterator>`, so that we could get error reports earlier. (I didn't give it a try, and it seems involving many changes.)
> > Why not to change the test so that you have a one broken test at once?
> > 
> > Case 1: Broken .symtab. Valid .dynsym.
> > Case 2: Valid .symtab. Broken .dynsym.
> > 
> > 
> * a one broken section
I think change test cases partly resolve this problem. I mean, if we one broken `.symtab`, and a good `.dynsym` section, consider the following situation.

```
# broken .symtab prevents us dumping .dynsym
$ llvm-nm --dynamic broken-symtab.elf
$ llvm-objdump --dynamic-syms broken-symtab.elf
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77864