[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]])
+
----------------
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.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77864