This is an archive of the discontinued LLVM Phabricator instance.

[Object/ELF] - Improve error reporting for notes.
ClosedPublic

Authored by grimar on Jul 10 2019, 12:52 AM.

Diff Detail

Event Timeline

grimar created this revision.Jul 10 2019, 12:52 AM
jhenderson added inline comments.Jul 10 2019, 4:38 AM
include/llvm/Object/ELF.h
205

I'm not sure making this llvm_unreachable is correct: if you are using libObject as a library, you could pass in an invalid Phdr, which should report an error, not an assertion.

225

Same comment about llvm_unreachable applies here.

test/tools/llvm-readobj/gnu-notes.test
1

of the notes -> of notes

24

Any particular reason you've changed these Offset values?

105

I don't know what consistency says here, but it was very off-putting to see size listed as decimal, given offset is in hex. I'd prefer both to be hex if possible.

149

Same comment here.

grimar marked an inline comment as done.Jul 10 2019, 4:45 AM
grimar added inline comments.
include/llvm/Object/ELF.h
205

Note that method name is notes_begin. I.e it expects to accept note Phdr and all
code we have do this check now.

I think that It is obviously a error in the code to pass Phdr not of type PT_NOTE here.
Showing such error to user is unuseful, code just should never do that first of all.

jhenderson added inline comments.Jul 10 2019, 4:51 AM
include/llvm/Object/ELF.h
205

Hmmm... okay, I'm not really convinced, but I don't feel strongly about it.

Either way, this can be simplified to assert(Phdr.p_type != ELF::PT_NOTE && "Phdr is not of type PT_NOTE").

MaskRay added inline comments.Jul 11 2019, 4:31 AM
include/llvm/Object/ELF.h
205

+1 for assert

225

I think llvm_unreachable should be fine. The precondition is that the section type should match.

How about

assert(Shdr.sh_type == ELF::SHT_NOTE && "Shdr is not of type SHT_NOTE");

MaskRay added inline comments.Jul 11 2019, 6:05 AM
test/tools/llvm-readobj/gnu-notes.test
24

I think that is because .text .data .bss are removed so the offset changes.

105

+1 for Twine::utohexstr(Phdr.p_filesz)

grimar updated this revision to Diff 209197.Jul 11 2019, 6:17 AM
grimar marked 12 inline comments as done.
grimar edited the summary of this revision. (Show Details)
  • Addressed review comments.
test/tools/llvm-readobj/gnu-notes.test
24

The original YAML contained redundant sections like .text, .eh_frame and also symbols. I cleaned it up and
that changed the offsets.

This revision is now accepted and ready to land.Jul 11 2019, 6:22 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2019, 6:49 AM
grimar reopened this revision.Jul 11 2019, 7:03 AM
This revision is now accepted and ready to land.Jul 11 2019, 7:03 AM
grimar planned changes to this revision.Jul 11 2019, 7:03 AM

Reverted in rL365779.

grimar added a comment.EditedJul 11 2019, 7:30 AM

It seems that my new test case triggered a unchecked error bug on bots.
I am going to recompile with LLVM_ENABLE_ABI_BREAKING_CHECKS to find the exact place.
Will reupload the fixed version after that.

grimar updated this revision to Diff 209288.Jul 11 2019, 12:14 PM
  • Fixed the reason of BB failture.

So the issue was in the original code. notes_begin() accepts a reference to
the Error object. When Error assignment operation happens it asserts with
LLVM_ENABLE_ABI_BREAKING_CHECKS because default Error::success() value
is never checked. Previously we just did not have a test case for such scenario
and so issue simply was never triggered.

This revision is now accepted and ready to land.Jul 11 2019, 12:14 PM
grimar requested review of this revision.Jul 11 2019, 12:14 PM

Could someone take a look again on the fix please.

MaskRay added inline comments.Jul 11 2019, 9:28 PM
include/llvm/Object/ELF.h
225

Alternatively,

assert(Shdr.sh_type == ELF::SHT_NOTE && "Shdr is not of type SHT_NOTE");
if (Err)
  return Elf_Note_Iterator(); // end() if Err is set
if (Shdr.sh_offset + Shdr.sh_size > getBufSize()) {
  ...
}
return Elf_Note_Iterator(base() + Shdr.sh_offset, Shdr.sh_size, Err);
grimar marked an inline comment as done.Jul 12 2019, 12:45 AM
grimar added inline comments.
include/llvm/Object/ELF.h
225

But that means that Err can be set to something before the call.
I do not think it is the behavior we want actually.
Our code written currently in the way that Err is always set to success before the call.
So we assume there is no unhandled error. But imagine we have one because of any reason.

Then

if (Err)
  return Elf_Note_Iterator(); // end() if Err is set

Will mark it as checked and that can hide the error.
In my version an assert will be triggered in that case, I think it is better. What do you think?

MaskRay added inline comments.Jul 12 2019, 1:17 AM
include/llvm/Object/ELF.h
225

In my version an assert will be triggered in that case, I think it is better. What do you think?

I agree. Wait, I think there might be a better version:

ErrorAsOutParameter ErrAsOutParam(&Err);

What I worry about assert(!Err && "Err should have Error::success() value"); is that it sets the check bit in an assertion build.

grimar updated this revision to Diff 209426.Jul 12 2019, 1:38 AM
grimar marked an inline comment as done.
  • Addressed review comment.
include/llvm/Object/ELF.h
225

What I worry about assert(!Err && "Err should have Error::success() value"); is that it sets the check bit in an assertion build.

True. This could be be solved if Error::getPtr() wasn't private: assert(Err.getPtr() && ..
I think in some cases it might be useful to have an ability to assert.

ErrorAsOutParameter

Cool, I did not know about this one, I used it, thanks!

MaskRay accepted this revision.Jul 12 2019, 1:47 AM
This revision is now accepted and ready to land.Jul 12 2019, 1:47 AM
This revision was automatically updated to reflect the committed changes.