[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:
> Higuoxing wrote:
> > 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
> > ```
> You can have
> 
> Case 3: Both .symtab and .dynsym are broken. The same behavior as in Case 1.
Ok, thanks a lot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77864