This is an archive of the discontinued LLVM Phabricator instance.

[Object/ELF.h] - Improve error reporting.
ClosedPublic

Authored by grimar on Jul 1 2019, 8:14 AM.

Details

Summary

The errors coming from ELF.h are usually not very useful because they are
uninformative. This patch is a first step to improve the situation.

  1. I tested this patch with a run of check-llvm and found that few messages are untested.

In this patch, I did not add more tests but marked all such cases with a "TODO" comment.

  1. For all tested messages I extended the error text to provide more details (see test cases changed).

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Jul 1 2019, 8:14 AM
grimar updated this revision to Diff 207325.Jul 1 2019, 8:18 AM
  • Cosmetic change.
grimar updated this revision to Diff 207327.Jul 1 2019, 8:26 AM
  • A bit more cosmetic.
MaskRay added inline comments.Jul 1 2019, 9:46 AM
include/llvm/Object/ELF.h
62 ↗(On Diff #207327)

&TableOrErr->front()

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

I absolutely would love this change.

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.

include/llvm/Object/ELF.h
58 ↗(On Diff #207327)

I'm not sure a static function here makes sense?

63 ↗(On Diff #207327)

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.

386 ↗(On Diff #207327)

has invalid -> has an invalid

Here and in other error messages that use this phrase.

394 ↗(On Diff #207327)

I think this would be better to be "... has invalid sh_size (0x1234) which is not a multiple of its sh_entsize of (..."

399 ↗(On Diff #207327)

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").

487 ↗(On Diff #207327)

Too small for what? Perhaps something like:

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

638 ↗(On Diff #207327)

Again, why is it invalid? Perhaps:

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

690 ↗(On Diff #207327)

sections string table -> section name string table

test/Object/corrupt.test
5 ↗(On Diff #207327)

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".

38 ↗(On Diff #207327)

Why has this lost its "warning: " check?

test/Object/invalid.test
155 ↗(On Diff #207327)

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.

238 ↗(On Diff #207327)

Why has this gained "error: " unlike the others? FWIW, I think it should be present, and should be added to the others.

MaskRay added inline comments.Jul 2 2019, 7:38 AM
include/llvm/Object/ELF.h
58 ↗(On Diff #207327)

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

grimar updated this revision to Diff 207561.Jul 2 2019, 7:42 AM
grimar marked 16 inline comments as done.
  • Addressed review comments.
include/llvm/Object/ELF.h
58 ↗(On Diff #207327)

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.

63 ↗(On Diff #207327)

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.

test/Object/invalid.test
238 ↗(On Diff #207327)

I added them for all tests in this file.

grimar added a comment.Jul 2 2019, 7:44 AM

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

I removed it. Honestly saying, I introduced it because for some reason was sure that Error keeps StringRef inside and not a std::string.

Overall I think this looks good. Others have been following the details far more closely that I've been so I'll defer to them.

include/llvm/Object/ELF.h
58 ↗(On Diff #207327)

Agreed. In general if it compiles and can be static you want it to be static (pending some edge cases).

grimar marked an inline comment as done.Jul 3 2019, 1:49 AM
grimar added inline comments.
include/llvm/Object/ELF.h
58 ↗(On Diff #207327)

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

MaskRay accepted this revision.Jul 3 2019, 2:13 AM
MaskRay added inline comments.
include/llvm/Object/ELF.h
63 ↗(On Diff #207327)

The use of consumeError deserves a comment (the comment of the function says it may indicate a design problem)

This revision is now accepted and ready to land.Jul 3 2019, 2:13 AM
grimar updated this revision to Diff 207729.EditedJul 3 2019, 2:26 AM
  • inline -> static inline
  • Addressed review 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.

include/llvm/Object/ELF.h
58 ↗(On Diff #207327)

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.

63 ↗(On Diff #207327)

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.

test/Object/invalid.test
48 ↗(On Diff #207561)

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.

430 ↗(On Diff #207561)

This should check the value too.

468 ↗(On Diff #207561)

Report/check the invalid size?

grimar marked an inline comment as done.Jul 3 2019, 3:09 AM

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.

lower-case for sure :) Because this is what we use in the linker. Would be strange to get a errors of a different styles from it.

include/llvm/Object/ELF.h
58 ↗(On Diff #207327)

Ok. After reading more about using inline with template functions, I agree with the part of your summary saying that "there's no point in using inline in this context". I am not sure about omitting the static, as makes possible to violate ODR and ELF.h is a global header, though I agree that having one more function with the same name is indeed very unlikely. Having that, I am OK to remove both inline and static.

jhenderson added inline comments.Jul 3 2019, 3:23 AM
include/llvm/Object/ELF.h
58 ↗(On Diff #207327)

Subtle ODR issues only come into play when people write a non-identical version of this function somewhere else (in the same namespace etc) not using ELF.h (if they are using ELF.h, they'll get a compiler error about duplicate definitions or something, I expect, and I think that's fine). I don't think it matters that much either way, and I don't care strongly whether we should have static here or not, so I'm happy to defer to others opinions.

grimar updated this revision to Diff 207749.Jul 3 2019, 3:39 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
include/llvm/Object/ELF.h
63 ↗(On Diff #207327)

I renamed it.

test/Object/invalid.test
48 ↗(On Diff #207561)

Yes, I noticed this place. There are many things I am planning to improve in the follow-ups for error-reporting, and this is one of them.

468 ↗(On Diff #207561)

No :) Because I am not changing this error message in this patch. (I only added a error: prefix to this test case).

grimar marked an inline comment as done.Jul 3 2019, 3:41 AM
grimar added inline comments.
test/Object/invalid.test
468 ↗(On Diff #207561)

(I am only touching ELF.h/ELF.cpp)

jhenderson added inline comments.Jul 3 2019, 3:47 AM
include/llvm/Object/ELF.h
59 ↗(On Diff #207749)

"for error"

60 ↗(On Diff #207749)

anyways actually -> really
Up to this place -> Before this point

61 ↗(On Diff #207749)

should already have called 'sections()' earlier -> should have called 'sections()'

62 ↗(On Diff #207749)

report -> reported

grimar updated this revision to Diff 208034.Jul 4 2019, 6:53 AM
grimar marked 4 inline comments as done.
  • Addressed review comments.
grimar edited the summary of this revision. (Show Details)Jul 4 2019, 6:58 AM
grimar added a comment.Jul 5 2019, 3:13 AM

So, can I land it, James?

jhenderson accepted this revision.Jul 5 2019, 3:20 AM

Yes, sorry. Somehow missed your last update.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2019, 4:29 AM