[PATCH] D64014: [Object/ELF.h] - Improve error reporting.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: include/llvm/Object/ELF.h:58
+template <class ELFT>
+static std::string getSecIndex(const ELFFile<ELFT> *Obj,
+                               const typename ELFT::Shdr *Sec) {
----------------
jhenderson wrote:
> I'm not sure a `static` function here makes sense?
I didn't want to split the definition and declaration.
(because it would require too many lines in a cpp for a template function:

```
template <class ELFT>
std::string getSecIndex(const ELFFile<ELFT> *Obj, ...) { 
...
}
...
template void object::getSecIndex<ELF32LE>();
template void object::getSecIndex<ELF32BE>();
template void object::getSecIndex<ELF64LE>();
template void object::getSecIndex<ELF64BE>();
)
```

And so there are two ways to do that correctly I think: either use
`inline` or `static`, otherwise the ODR would be violated.

It is probably can be `inline` (like other helpers in this file), I did that change.


================
Comment at: include/llvm/Object/ELF.h:63
+    return "[" + std::to_string(Sec - &(*TableOrErr).front()) + "]";
+  llvm::consumeError(TableOrErr.takeError());
+  return "[?]";
----------------
jhenderson wrote:
> Why not have this return an Expected, and let the call sites handle the error rather than throwing it away here? Of course, they could throw it away there.
Two reasons:
1) To simplify the implementation. With the current version it is a very convenient helper that does not need additional wrapping.
     It also allows to build the final string of view `[index X]` in one place.
2) I believe this error will *never* happen actually. Because our code always calls `sections()` earlier than this method is called,
so it just should succeed here.
But I still had to handle the error somehow and decided that because of all above the best way is just to return "[?]" as a section index in this
really unlikely and untestable (I believe) case.



================
Comment at: test/Object/invalid.test:238
 
-# INVALID-EXT-SYMTAB-INDEX: index past the end of the symbol table
+# INVALID-EXT-SYMTAB-INDEX: error: extended symbol index (0) is past the end of the SHT_SYMTAB_SHNDX section of size 0
 
----------------
jhenderson wrote:
> Why has this gained "error: " unlike the others? FWIW, I think it should be present, and should be added to the others.
I added them for all tests in this file.


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

https://reviews.llvm.org/D64014