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

Wed Jul 3 02:37:06 PDT 2019

jhenderson added a comment.

There are a number of the error messages which you are updating that use upper-case for the first letter, but most others use lower-case. I think we should standardize on one, although it's not immediately obvious to me which it should be.

================
Comment at: include/llvm/Object/ELF.h:58
+template <class ELFT>
+static std::string getSecIndex(const ELFFile<ELFT> *Obj,
+                               const typename ELFT::Shdr *Sec) {
----------------
grimar wrote:
> jakehehrlich wrote:
> > MaskRay wrote:
> > > grimar wrote:
> > > > 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.
> > > It makes sense. If static is used, the instances will get internal linkage and defining the template function with the same name in another translation unit will not cause a conflict (ODR violation).
> > Agreed. In general if it compiles and can be static you want it to be static (pending some edge cases).
> I found this article about "static inline vs inline vs static in C++": https://gist.github.com/htfy96/50308afc11678d2e3766a36aa60d5f75
> And it turns out my statement was not fully correct. I assumed that inline implies static, but it is actually not.
> 
> Going to change `inline std::string getSecIndex` to `static inline std::string getSecIndex`
I've been refreshing my memory of `inline`, `static` and template functions since making this statement, and turns out I was wrong (slightly). Typing it out to make sure I have got my understanding correct as much as anything else:
1) `static` means you'll get a copy of this function in every translation unit, local to that translation unit (or it might be inlined).
2) `inline` means there'll be a copy in every TU. If there's one or more other instances of the function, they have to be identical, and one will end up being used. If they aren't identical, it is undefined behaviour.
3) Template function definitions work the same way as `inline` functions: one copy is instantiated in each TU that uses it. If there's one or more other instances of the function, they have to be identical, and if so, one will be used. If they are not identical it is undefined behaviour.

>From this I conclude that there's no point in using `inline` in this context. There is a point in using `static` if we think somewhere else in the code-base there's a TU that doesn't reference this header file, where somebody creates an identical copy of the function signature, but with different contents. If we don't care about that situation, then I'd argue that `static` is bad because it will increase executable size (assuming it isn't inlined) more than a template function definition otherwise would. It's not obvious to me that this function would be inlined (I expect it would, but it might not be), and I don't believe any other instances of this function are likely to be defined elsewhere in the `object` namespace, so personally I don't believe either `static` or `inline` should be used in this case. The same may not be true in other cases though, and I don't care that much about it.

Don't forget, `inline` may or may not have an impact on inlining of the function. The compiler will make the decision regardless of its presence or not.

================
Comment at: include/llvm/Object/ELF.h:63
+    return "[" + std::to_string(Sec - &(*TableOrErr).front()) + "]";
+  llvm::consumeError(TableOrErr.takeError());
+  return "[?]";
----------------
MaskRay wrote:
> grimar wrote:
> > 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.
> > 
> The use of consumeError deserves a comment (the comment of the function says it may indicate a design problem)
Whilst what you're saying is fine for now, I can imagine in the future that people will want to call this function in other contexts beyond error handling. As such, it needs future proofing. At least a comment at the signature level saying that any unknown indexes are ignored would be good or changing the function name to something like "getSecIndexForError" would be good.

================
Comment at: test/Object/invalid.test:48

-# RELA: Section is not SHT_RELA
+# RELA: LLVM ERROR: Section is not SHT_RELA

----------------
Not to be fixed now, but it would be great if this didn't use report_fatal_error, since it's not really an internal error in most cases.

================
Comment at: test/Object/invalid.test:430

-# PHENTSIZE: invalid e_phentsize
+# PHENTSIZE: error: invalid e_phentsize

----------------
This should check the value too.

================
Comment at: test/Object/invalid.test:468

-# RELOC-BROKEN-ENTSIZE: Invalid entity size
+# RELOC-BROKEN-ENTSIZE: error: Invalid entity size

----------------
Report/check the invalid size?

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

https://reviews.llvm.org/D64014