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

[George Rimar via Phabricator via llvm-commits]

grimar 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:
> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77864