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

[grimar]

Summary

This patch improves the error messages reported for
note sections and phdrs and also makes a cleanup for
existent test case.

[jhenderson]

include/llvm/Object/ELF.h

205 ↗

(On Diff #208886)

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 ↗

(On Diff #208886)

Same comment about llvm_unreachable applies here.

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

1 ↗

(On Diff #208886)

of the notes -> of notes

24 ↗

(On Diff #208886)

Any particular reason you've changed these Offset values?

105 ↗

(On Diff #208886)

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 ↗

(On Diff #208886)

Same comment here.

[grimar]

include/llvm/Object/ELF.h

205 ↗

(On Diff #208886)

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]

include/llvm/Object/ELF.h

205 ↗

(On Diff #208886)

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]

include/llvm/Object/ELF.h

205 ↗

(On Diff #208886)

+1 for assert

225 ↗

(On Diff #208886)

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]

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

24 ↗

(On Diff #208886)

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

105 ↗

(On Diff #208886)

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

[grimar]

• Addressed review comments.

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

24 ↗

(On Diff #208886)

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

[jhenderson]

LGTM.

[grimar]

Reverted in rL365779.

[grimar]

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]

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

[grimar]

Could someone take a look again on the fix please.

[MaskRay]

include/llvm/Object/ELF.h

225 ↗

(On Diff #209288)

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]

include/llvm/Object/ELF.h

225 ↗

(On Diff #209288)

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]

include/llvm/Object/ELF.h

225 ↗

(On Diff #209288)

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]

• Addressed review comment.

include/llvm/Object/ELF.h

225 ↗

(On Diff #209288)

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!

llvm/trunk/include/llvm/Object/ELF.h

public:
   /// Get an iterator over notes in a program header.
   ///
   /// The program header must be of type \c PT_NOTE.
   ///
   /// \param Phdr the program header to iterate over.
   /// \param Err [out] an error to support fallible iteration, which should
   ///  be checked after iteration ends.
   Elf_Note_Iterator notes_begin(const Elf_Phdr &Phdr, Error &Err) const {
-    if (Phdr.p_type != ELF::PT_NOTE) {
-      // TODO: this error is untested.
-      Err = createError("attempt to iterate notes of non-note program header");
-      return Elf_Note_Iterator(Err);
-    }
+    assert(Phdr.p_type == ELF::PT_NOTE && "Phdr is not of type PT_NOTE");
+    ErrorAsOutParameter ErrAsOutParam(&Err);
     if (Phdr.p_offset + Phdr.p_filesz > getBufSize()) {
-      // TODO: this error is untested.
-      Err = createError("invalid program header offset/size");
+      Err = createError("PT_NOTE header has invalid offset (0x" +
+                        Twine::utohexstr(Phdr.p_offset) + ") or size (0x" +
+                        Twine::utohexstr(Phdr.p_filesz) + ")");
       return Elf_Note_Iterator(Err);
     }
     return Elf_Note_Iterator(base() + Phdr.p_offset, Phdr.p_filesz, Err);
   }
 
   /// Get an iterator over notes in a section.
   ///
   /// The section must be of type \c SHT_NOTE.
   ///
   /// \param Shdr the section to iterate over.
   /// \param Err [out] an error to support fallible iteration, which should
   ///  be checked after iteration ends.
   Elf_Note_Iterator notes_begin(const Elf_Shdr &Shdr, Error &Err) const {
-    if (Shdr.sh_type != ELF::SHT_NOTE) {
-      // TODO: this error is untested.
-      Err = createError("attempt to iterate notes of non-note section");
-      return Elf_Note_Iterator(Err);
-    }
+    assert(Shdr.sh_type == ELF::SHT_NOTE && "Shdr is not of type SHT_NOTE");
+    ErrorAsOutParameter ErrAsOutParam(&Err);
     if (Shdr.sh_offset + Shdr.sh_size > getBufSize()) {
-      // TODO: this error is untested.
-      Err = createError("invalid section offset/size");
+      Err = createError("SHT_NOTE section " + getSecIndexForError(this, &Shdr) +
+                        " has invalid offset (0x" +
+                        Twine::utohexstr(Shdr.sh_offset) + ") or size (0x" +
+                        Twine::utohexstr(Shdr.sh_size) + ")");
       return Elf_Note_Iterator(Err);
     }
     return Elf_Note_Iterator(base() + Shdr.sh_offset, Shdr.sh_size, Err);
   }
 
   /// Get the end iterator for notes.
   Elf_Note_Iterator notes_end() const {
     return Elf_Note_Iterator();

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

-# RUN: yaml2obj %s > %t.so
-# RUN: llvm-readelf --notes %t.so | FileCheck %s --check-prefix=GNU
-# RUN: llvm-readobj --elf-output-style LLVM --notes %t.so | FileCheck %s --check-prefix=LLVM
+## Test tools are able to dump different types of notes.
 
-# GNU:      Displaying notes found at file offset 0x00000340 with length 0x00000020:
+# RUN: yaml2obj --docnum=1 %s > %t1.so
+# RUN: llvm-readelf --notes %t1.so | FileCheck %s --check-prefix=GNU
+# RUN: llvm-readobj --notes %t1.so | FileCheck %s --check-prefix=LLVM
+
+# GNU:      Displaying notes found at file offset 0x00000200 with length 0x00000020:
 # GNU-NEXT:   Owner                 Data size       Description
 # GNU-NEXT:   GNU                   0x00000010      NT_GNU_ABI_TAG (ABI version tag)
 # GNU-NEXT:     OS: Linux, ABI: 2.6.32
 
-# GNU:      Displaying notes found at file offset 0x00000360 with length 0x00000020:
+# GNU:      Displaying notes found at file offset 0x00000220 with length 0x00000020:
 # GNU-NEXT:   Owner                 Data size       Description
 # GNU-NEXT:   GNU                  0x00000010       NT_GNU_BUILD_ID (unique build ID bitstring)
 # GNU-NEXT:     Build ID: 4fcb712aa6387724a9f465a32cd8c14b
 
-# GNU:      Displaying notes found at file offset 0x000003cc with length 0x0000001c:
+# GNU:      Displaying notes found at file offset 0x00000240 with length 0x0000001c:
 # GNU-NEXT:   Owner                 Data size       Description
 # GNU-NEXT:   GNU                  0x00000009       NT_GNU_GOLD_VERSION (gold version)
 # GNU-NEXT:     Version: gold 1.11
 
 # LLVM:      Notes [
 # LLVM-NEXT:   NoteSection {
-# LLVM-NEXT:     Offset: 0x340
+# LLVM-NEXT:     Offset: 0x200
 # LLVM-NEXT:     Size: 0x20
 # LLVM-NEXT:     Note {
 # LLVM-NEXT:       Owner: GNU
 # LLVM-NEXT:       Data size: 0x10
 # LLVM-NEXT:       Type: NT_GNU_ABI_TAG (ABI version tag)
 # LLVM-NEXT:       OS: Linux
 # LLVM-NEXT:       ABI: 2.6.32
 # LLVM-NEXT:     }
 # LLVM-NEXT:   }
 # LLVM-NEXT:   NoteSection {
-# LLVM-NEXT:     Offset: 0x360
+# LLVM-NEXT:     Offset: 0x220
 # LLVM-NEXT:     Size: 0x20
 # LLVM-NEXT:     Note {
 # LLVM-NEXT:       Owner: GNU
 # LLVM-NEXT:       Data size: 0x10
 # LLVM-NEXT:       Type: NT_GNU_BUILD_ID (unique build ID bitstring)
 # LLVM-NEXT:       Build ID: 4fcb712aa6387724a9f465a32cd8c14b
 # LLVM-NEXT:     }
 # LLVM-NEXT:   }
 # LLVM-NEXT:   NoteSection {
-# LLVM-NEXT:     Offset: 0x3CC
+# LLVM-NEXT:     Offset: 0x240
 # LLVM-NEXT:     Size: 0x1C
 # LLVM-NEXT:     Note {
 # LLVM-NEXT:       Owner: GNU
 # LLVM-NEXT:       Data size: 0x9
 # LLVM-NEXT:       Type: NT_GNU_GOLD_VERSION (gold version)
 # LLVM-NEXT:       Version: gold 1.11
 # LLVM-NEXT:     }
 # LLVM-NEXT:   }
 # LLVM-NEXT: ]
 
 --- !ELF
 FileHeader:
-  Class:           ELFCLASS64
-  Data:            ELFDATA2LSB
-  Type:            ET_EXEC
-  Machine:         EM_X86_64
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
 Sections:
-  - Name:            .note.ABI-tag
-    Type:            SHT_NOTE
-    AddressAlign:    0x0000000000000004
-    Content:         040000001000000001000000474E550000000000020000000600000020000000
-  - Name:            .note.gnu.build-id
-    Type:            SHT_NOTE
-    Flags:           [ SHF_ALLOC ]
-    Address:         0x0000000000400120
-    AddressAlign:    0x0000000000000004
-    Content:         040000001000000003000000474E55004FCB712AA6387724A9F465A32CD8C14B
-  - Name:            .text
-    Type:            SHT_PROGBITS
-    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
-    Address:         0x0000000000400140
-    AddressAlign:    0x0000000000000001
-    Content:         31C0C3
-  - Name:            .eh_frame
-    Type:            SHT_PROGBITS
-    Flags:           [ SHF_ALLOC ]
-    Address:         0x0000000000400148
-    AddressAlign:    0x0000000000000008
-    Content:         1400000000000000017A5200017810011B0C070890010000140000001C000000D8FFFFFF030000000000000000000000
-  - Name:            .data
-    Type:            SHT_PROGBITS
-    Flags:           [ SHF_WRITE, SHF_ALLOC ]
-    Address:         0x0000000000401000
-    AddressAlign:    0x0000000000000001
-    Content:         ''
-  - Name:            .bss
-    Type:            SHT_NOBITS
-    Flags:           [ SHF_WRITE, SHF_ALLOC ]
-    Address:         0x0000000000401000
-    AddressAlign:    0x0000000000000001
-  - Name:            .comment
-    Type:            SHT_PROGBITS
-    Flags:           [ SHF_MERGE, SHF_STRINGS ]
-    AddressAlign:    0x0000000000000001
-    Content:         004743433A2028474E552920352E342E3000
-  - Name:            .note.gnu.gold-version
-    Type:            SHT_NOTE
-    AddressAlign:    0x0000000000000004
-    Content:         040000000900000004000000474E5500676F6C6420312E3131000000
-Symbols:
-  - Name:            reduced.c
-    Type:            STT_FILE
-  - Type:            STT_FILE
-  - Name:            main
-    Type:            STT_FUNC
-    Section:         .text
-    Value:           0x0000000000400140
-    Size:            0x0000000000000003
-    Binding: STB_GLOBAL
-  - Name:            _edata
-    Value:           0x0000000000401000
-    Binding: STB_GLOBAL
-  - Name:            __bss_start
-    Value:           0x0000000000401000
-    Binding: STB_GLOBAL
-  - Name:            _end
-    Value:           0x0000000000401000
-    Binding: STB_GLOBAL
-...
+  - Name:         .note.ABI-tag
+    Type:         SHT_NOTE
+    AddressAlign: 0x0000000000000004
+    Content:      040000001000000001000000474E550000000000020000000600000020000000
+  - Name:         .note.gnu.build-id
+    Type:         SHT_NOTE
+    Flags:        [ SHF_ALLOC ]
+    Address:      0x0000000000400120
+    AddressAlign: 0x0000000000000004
+    Content:      040000001000000003000000474E55004FCB712AA6387724A9F465A32CD8C14B
+  - Name:         .note.gnu.gold-version
+    Type:         SHT_NOTE
+    AddressAlign: 0x0000000000000004
+    Content:      040000000900000004000000474E5500676F6C6420312E3131000000
+
+## Test tools report an error if a note section has an invalid offset
+## that goes past the end of file.
+
+# RUN: yaml2obj --docnum=2 %s > %t2.so
+# RUN: not llvm-readelf --notes %t2.so 2>&1 | FileCheck %s --check-prefix=ERR1
+# RUN: not llvm-readobj --notes %t2.so 2>&1 | FileCheck %s --check-prefix=ERR1
+
+# ERR1: error: SHT_NOTE section [index 1] has invalid offset (0xffff0000) or size (0x0)
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:     .note
+    Type:     SHT_NOTE
+    ShOffset: 0xffff0000
+
+## Test tools report an error if a note section has invalid size
+## that goes past the end of file.
+
+# RUN: yaml2obj --docnum=3 %s > %t3.so
+# RUN: not llvm-readelf --notes %t3.so 2>&1 | FileCheck %s --check-prefix=ERR2
+# RUN: not llvm-readobj --notes %t3.so 2>&1 | FileCheck %s --check-prefix=ERR2
+
+# ERR2: error: SHT_NOTE section [index 1] has invalid offset (0x180) or size (0xffff0000)
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:   .note
+    Type:   SHT_NOTE
+    ShSize: 0xffff0000
+
+## Test tools report an error if a note program header has an invalid offset that
+## goes past the end of file.
+
+# RUN: yaml2obj --docnum=4 %s > %t4.so
+# RUN: not llvm-readelf --notes %t4.so 2>&1 | FileCheck %s --check-prefix=ERR3
+# RUN: not llvm-readobj --notes %t4.so 2>&1 | FileCheck %s --check-prefix=ERR3
+
+# ERR3: error: PT_NOTE header has invalid offset (0xffff0000) or size (0x0)
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_CORE
+  Machine: EM_X86_64
+Sections:
+  - Name: .note
+    Type: SHT_NOTE
+ProgramHeaders:
+  - Type: PT_NOTE
+    Offset: 0xffff0000
+    Sections:
+      - Section: .note
+
+## Test tools report an error if a note program header has an invalid size that
+## goes past the end of file.
+
+# RUN: yaml2obj --docnum=5 %s > %t5.so
+# RUN: not llvm-readelf --notes %t5.so 2>&1 | FileCheck %s --check-prefix=ERR4
+# RUN: not llvm-readobj --notes %t5.so 2>&1 | FileCheck %s --check-prefix=ERR4
+
+# ERR4: error: PT_NOTE header has invalid offset (0x1b8) or size (0xffff0000)
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_CORE
+  Machine: EM_X86_64
+Sections:
+  - Name: .note
+    Type: SHT_NOTE
+ProgramHeaders:
+  - Type: PT_NOTE
+    FileSize: 0xffff0000
+    Sections:
+      - Section: .note