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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I absolutely would love this change.

In D64014#1565714 <https://reviews.llvm.org/D64014#1565714>, @jakehehrlich wrote:

> How important is using ErrSaver. Isn't it possible to just use std::string in these error cases?


What @jakehehrlich said. I don't think we need to worry about performance here, since we are throwing errors, so really it should be whatever's the simplest way to get the error back up the stack. I'm also not keen on introducing any form of global state if it can be avoided, especially in libraries.



================
Comment at: include/llvm/Object/ELF.h:58
+template <class ELFT>
+static std::string getSecIndex(const ELFFile<ELFT> *Obj,
+                               const typename ELFT::Shdr *Sec) {
----------------
I'm not sure a `static` function here makes sense?


================
Comment at: include/llvm/Object/ELF.h:63
+    return "[" + std::to_string(Sec - &(*TableOrErr).front()) + "]";
+  llvm::consumeError(TableOrErr.takeError());
+  return "[?]";
----------------
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.


================
Comment at: include/llvm/Object/ELF.h:386
+        ErrSaver.save("section " + getSecIndex(this, Sec) +
+                      " has invalid sh_entsize: " + Twine(Sec->sh_entsize)));
 
----------------
has invalid -> has an invalid

Here and in other error messages that use this phrase.


================
Comment at: include/llvm/Object/ELF.h:394
+                                     " has invalid sh_size (" + Twine(Size) +
+                                     "): is not a multiple of sh_entsize (" +
+                                     Twine(Sec->sh_entsize) + ")"));
----------------
I think this would be better to be "... has invalid sh_size (0x1234) which is not a multiple of its sh_entsize of (..."


================
Comment at: include/llvm/Object/ELF.h:399
+    return createError(ErrSaver.save(
+        "section " + getSecIndex(this, Sec) + " has invalid sh_offset (0x" +
+        Twine::utohexstr(Offset) + ") or sh_size (" + Twine(Size) + ")"));
----------------
This should say WHY the sh_offset/sh_size is invalid (possibly saying something like "has a sh_offset + sh_size that cannot be represented").


================
Comment at: include/llvm/Object/ELF.h:487
   if (sizeof(Elf_Ehdr) > Object.size())
-    return createError("Invalid buffer");
+    return createError(ErrSaver.save("invalid buffer, size is too small: " +
+                                     Twine(Object.size())));
----------------
Too small for what? Perhaps something like:

"invalid buffer: the size (Object.size()) is smaller than an ELF header (sizeof(Elf_Ehdr))".


================
Comment at: include/llvm/Object/ELF.h:638
+    return createError(
+        ErrSaver.save("SHT_SYMTAB_SHNDX section has invalid sh_size: " +
+                      Twine(SymTable.sh_size)));
----------------
Again, why is it invalid? Perhaps:

"SHT_SYMTAB_SHNDX section has sh_size (0x1234) which is not equal to the number of symbols (0x42)".


================
Comment at: include/llvm/Object/ELF.h:690
+                                     ") offset which goes past the end of the "
+                                     "sections string table"));
   return StringRef(DotShstrtab.data() + Offset);
----------------
sections string table -> section name string table


================
Comment at: test/Object/corrupt.test:5
 
-SECNAME: invalid string offset
+SECNAME: a section [5] has invalid sh_name (0x100031) offset which goes past the end of the sections string table
 
----------------
Here and in other cases, I'm not sure it's clear enough to me that [5] is a section index. Perhaps it could be "[index 5]" or "a section with index [5]" or simply "a section with index 5".


================
Comment at: test/Object/corrupt.test:38
 
-VIRTADDR: warning: Unable to parse DT_STRTAB: Virtual address is not in any segment
+VIRTADDR: Unable to parse DT_STRTAB: virtual address is not in any segment: 0xffff
 
----------------
Why has this lost its "warning: " check?


================
Comment at: test/Object/invalid.test:155
 
-# INVALID-SYMTAB-LINK: invalid section index
+# INVALID-SYMTAB-LINK: invalid section index: 255
 
----------------
This is an improvement, but I think the call site will need improving too to report where this section index is being referenced. Doesn't need to be in this change though.


================
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
 
----------------
Why has this gained "error: " unlike the others? FWIW, I think it should be present, and should be added to the others.


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

https://reviews.llvm.org/D64014