[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:
> 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.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77864