llvm-objdump can error out with an unexpected EOF error if .bss is larger than the file being dumped.

[sidneym]

Summary

llvm-objdump -D this file:

int a[100000];

int
main() {
  return 0;
}

Will produce an error, "The end of the file was unexpectedly encountered"

This happens because of a check in Binary.h checkOffset. (Addr + Size > M.getBufferEnd()).

Since the .bss section doesn't occupy space in the file this check fails when the above program is dumped with -D.

This change avoids the reading .bss sections.

I added a test for X86_64 but this isn't target specific.

[MaskRay]

inline Expected<StringRef> SectionRef::getContents() const { is called 4 times in llvm-objdump.cpp. They probably should all be replaced. The method should special case SHT_NOBITS - the sh_offset field of a SHT_NOBITS section is ignored. For a GNU ld linked binary, sh_offset may be greater than the file size (lld does not do this).

Please don't check in a prebuilt binary. I think we can construct a test with yaml2obj.

[grimar]

llvm/test/tools/llvm-objdump/X86/bss.test

1 ↗

(On Diff #225683)

Use ## for comments (this is inline with most of the new tests we introduce for llvm tools like this one).

[sidneym]

Changed getContents to return a zero-length array when the section type was NOBITS.

I looked at the other locations in that file and they all seemed to be set up to avoid dumping bss but not the potential of a non-bss/nobits section. Adding this to getContents should address those sections and usages by other tools.

I've never used yaml before so this might not be the perfect test but it sure feels better than adding binaries to the test tree.

[grimar]

The code change looks good to me.
Comments/suggestions are below.

llvm/test/tools/llvm-objdump/X86/bss.test

1 ↗

(On Diff #225935)

"bss.test" is probably not a goodname.
I'd suggest "disassemble-bss.test" (it is similar to other disassemble-*.test we have).

4 ↗

(On Diff #225935)

I'd probably change this. Instead of testing the error message which might change
at any time, you can check that .bss is disassembled:

# CHECK: Disassembly of section .bss:
# CHECK: 0000000000000000 .bss:
# CHECK-NEXT:		...

I'd also don't optimize the invocations that much and use files:

yaml2obj %s -o %t
llvm-objdump -D %t | FileCheck %s

It helps to debug tests that fails. With that it is easy to copy paste the
invocation line from the output and have a file created for a futher inspection.

5 ↗

(On Diff #225935)

nit: add an empty line between check and yaml.

25 ↗

(On Diff #225935)

You can simplify to:

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_EXEC
  Machine: EM_X86_64
Sections:
  - Name:  .bss
    Type:  SHT_NOBITS
    Flags: [ SHF_WRITE, SHF_ALLOC ]
    Size:  0x0000000000001000

[sidneym]

Rename and update test.

[MaskRay]

llvm/include/llvm/Object/ELFObjectFile.h

727

Delete + sh_offset to solve the problem I described in a previous comment.

[grimar]

llvm/include/llvm/Object/ELFObjectFile.h

727

It is strange to know that sh_offset can be greater than the file size.

If we want to have this change and not error out, I'd add a comment and a one more test case for
this (yaml2obj allows overriding the sh_offset with use of "ShOffset" field).

llvm/test/tools/llvm-objdump/X86/elf-disassemble-bss.test

17

As I mentioned earlier, you do not need this, Address and AddressAlign fields below.

[sidneym]

llvm/include/llvm/Object/ELFObjectFile.h

727

I don't think sh_offset will be larger than the file size but sh_offset+sh_size can be and that would cause checkOffset to fail. I guess one could argue that if sh_offset is outside the scope of the file something is really bogus about the file but if the section NOBITS maybe it doesn't matter.

[MaskRay]

LG after grimar's comment is addressed.

llvm/include/llvm/Object/ELFObjectFile.h

727

It can be larger than the file size with some usage of objcopy. We can proceed with this change for now. I'll come up a test later.

[sidneym]

Remove +sh_offset update testcase to include a test for ShOffset > file size.

[sidneym]

Forgot to remove Address/AddressAlign

[MaskRay]

Thanks for the update. sh_offset > file size is what GNU objcopy may do. LG but please wait for others' opinions.

[bcain]

In D69192#1721507, @MaskRay wrote:

Thanks for the update. sh_offset > file size is what GNU objcopy may do. LG but please wait for others' opinions.

LGTM

[grimar]

LGTM.

[MaskRay]

llvm/test/tools/llvm-objdump/X86/elf-disassemble-bss.test

33

Delete Entry

[sidneym]

Remove Entry: from testcase

[sidneym]

Can one of the reviewers push this? I lost my commit access, waiting for it to be restored.

Thanks.

[MaskRay]

In D69192#1734389, @sidneym wrote:

Can one of the reviewers push this? I lost my commit access, waiting for it to be restored.

Thanks.

Will do that shortly.

llvm/include/llvm/Object/ELFObjectFile.h

 uint64_t ELFObjectFile<ELFT>::getSectionSize(DataRefImpl Sec) const {
   return getSection(Sec)->sh_size;
 }
 
 template <class ELFT>
 Expected<ArrayRef<uint8_t>>
 ELFObjectFile<ELFT>::getSectionContents(DataRefImpl Sec) const {
   const Elf_Shdr *EShdr = getSection(Sec);
+  if (EShdr->sh_type == ELF::SHT_NOBITS)
+    return makeArrayRef((const uint8_t *)base(), 0);
   if (std::error_code EC =
           checkOffset(getMemoryBufferRef(),
                       (uintptr_t)base() + EShdr->sh_offset, EShdr->sh_size))
     return errorCodeToError(EC);
   return makeArrayRef((const uint8_t *)base() + EShdr->sh_offset,
                       EShdr->sh_size);
 }
 

llvm/test/tools/llvm-objdump/X86/elf-disassemble-bss.test

+## Check that when BSS is larger than the file llvm-objdump doesn't
+## assert with an unexpected end of file error.
+# RUN: yaml2obj --docnum=1 %s > %t
+# RUN: yaml2obj --docnum=2 %s > %t.2
+# RUN: llvm-objdump -D %t | FileCheck %s
+# RUN: llvm-objdump -D %t.2 | FileCheck %s
+
+# CHECK: Disassembly of section .bss:
+# CHECK: .bss:
+# CHECK-NEXT: ...
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .bss
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    Size:            0x0000000000001000
+...
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_EXEC
+  Machine:         EM_X86_64
+Sections:
+  - Name:            .bss
+    Type:            SHT_NOBITS
+    Flags:           [ SHF_WRITE, SHF_ALLOC ]
+    Size:            0x0000000000001000
+    ShOffset:        0x0000000080000000
+...