[llvm-objdump] Don't throw error for empty dynamic section

[MaskRay]

Summary

objcopy --only-keep-debug flag creates files with empty dynamic section. llvm-objdump should not throw error for these files.

[MaskRay]

I would argue that this is indeed an invalid case, though objdump -p does not error.

http://www.sco.com/developers/gabi/latest/ch5.dynamic.html#dynamic_section

Except for the DT_NULL element at the end of the array, and the relative order of DT_NEEDED elements, entries may appear in any order. Tag values not appearing in the table are reserved.

I take it that there should be a DT_NULL element at the end, so this section cannot be empty. The if (Dyn.back().d_tag != ELF::DT_NULL) test below becomes slightly more complex: "dynamic section must end with a DT_NULL" to "non-empty dynamic section must end with a DT_NULL".

That said, -p emitting an error may be a bit inconvenient because -x otherwise does not emit any error.

Maybe we should test: !empty or SHT_NOBITS?

BTW, do you have an opinion on D67090?

llvm/test/tools/llvm-objdump/elf-keep-debug-only.test

1

We can move this test into elf-dynamic-section.test or rename it to elf-dynamic-section-*.test.

3

-o

16

For --only-keep-debug, this is SHT_NOBITS

[grimar]

llvm/lib/Object/ELF.cpp

541

DT_NULL is a mandatory field according to the spec. I think it is invalid to remove if (Dyn.empty()) check here.
lib/Object is a library that is used not only by llvm-objdump, but by many our tools.
For example, dynamicEntries() is also used by llvm-elfabi tool (though I am not familar with it, but anyways,
I guess it does not want to accept an invalid dynamic section).

Perhaps, you should add a flag like bool AllowEmpty or do a change on a llvm-objdump side instead.

[grimar]

llvm/test/tools/llvm-objdump/elf-keep-debug-only.test

22

I have an error when trying to use this YAML as is:
"yaml2obj: error: unknown section referenced: '.dynstr' by program header"

(I've added a dummy .dynstr declaration to procceed)

[jhenderson]

In D68249#1689097, @MaskRay wrote:

I would argue that this is indeed an invalid case, though objdump -p does not error.

http://www.sco.com/developers/gabi/latest/ch5.dynamic.html#dynamic_section

Except for the DT_NULL element at the end of the array, and the relative order of DT_NEEDED elements, entries may appear in any order. Tag values not appearing in the table are reserved.

I take it that there should be a DT_NULL element at the end, so this section cannot be empty. The if (Dyn.back().d_tag != ELF::DT_NULL) test below becomes slightly more complex: "dynamic section must end with a DT_NULL" to "non-empty dynamic section must end with a DT_NULL".

That said, -p emitting an error may be a bit inconvenient because -x otherwise does not emit any error.

Maybe we should test: !empty or SHT_NOBITS?

BTW, do you have an opinion on D67090?

I agree that an empty dynamic table is strictly illegal, but also that it's not ideal that we emit an error here for this specific case. However, I'm not sure testing NOBITS will work, because in this case, the range is taken from the PT_DYNAMIC program header, if I'm not mistaken. In fact, my reading of the code is that the following happens when a SHT_NOBITS .dynamic section is present.

Assumed file layout:

PT_DYNAMIC p_filesz = 0
.dynamic SHT_NOBITS

1. Search for PT_DYNAMIC segment. Find it.
2. Load data from the segment. Resultant Dyn is empty (because p_filesz is 0).
3. Dyn is empty, so attempt to load data from section.
4. No SHT_DYNAMIC section is found, so Dyn remains empty.
5. Dyn is empty, so emit an error.

As an aside, the dynamicEntries code will emit an error if there is no dynamic data found at all (i.e. there is no PT_DYNMIC or SHT_DYNAMIC section). I'd argue that this is incorrect (if neither is found, an empty range should be returned). However, that's somewhat irrelevant to the case at hand, since the PT_DYNAMIC is still present.

llvm/lib/Object/ELF.cpp

541

As empty dynamic sections are invalid, perhaps the best thing to do is to capture the errors within llvm-objdump code and report them as warnings instead. Not sure about that though, as it would mean that a user would always get a warning if they wanted to do -x on the output of --only-keep-debug.

llvm/test/tools/llvm-objdump/elf-keep-debug-only.test

1

I'd go with elf-dyanmic-section-empty.test.

A lot of the newer tests in the LLVM binutils use '##' to mark comments, to distinguish them from lit RUN: and FileCheck CHECK: lines. It would be good if you do that here too.

[raj.khem]

I have submitted a github issue which seems to be related here https://github.com/llvm/llvm-project/issues/54456

[MaskRay]

In D68249#3408222, @raj.khem wrote:

I have submitted a github issue which seems to be related here https://github.com/llvm/llvm-project/issues/54456

Created D122505 as an alternative. I think we should keep the diagnostic for an invalid empty dynamic section.

llvm/lib/Object/ELF.cpp

for (const Elf_Shdr &Sec : *SectionsOrError) {
         break;
       }
     }
 
     if (!Dyn.data())
       return ArrayRef<Elf_Dyn>();
   }
 
-  if (Dyn.empty())
-    // TODO: this error is untested.
-    return createError("invalid empty dynamic section");
-
+  if (!Dyn.empty()) {
     if (DynSecSize % sizeof(Elf_Dyn) != 0)
       // TODO: this error is untested.
       return createError("malformed dynamic section");
 
     if (Dyn.back().d_tag != ELF::DT_NULL)
       // TODO: this error is untested.
       return createError("dynamic sections must be DT_NULL terminated");
+  }
 
   return Dyn;
 }
 
 template <class ELFT>
 Expected<const uint8_t *> ELFFile<ELFT>::toMappedAddr(uint64_t VAddr) const {
   auto ProgramHeadersOrError = program_headers();
   if (!ProgramHeadersOrError)

llvm/test/tools/llvm-objdump/elf-keep-debug-only.test

+# 'objcopy --only-keep-debug' will create objects with empty dynamic section,
+# llvm-objcopy should not crash for these files.
+# RUN: yaml2obj %s > %t
+# RUN: llvm-objdump -p %t | FileCheck %s
+
+# CHECK:      Dynamic Section:
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_EXEC
+  Machine: EM_X86_64
+Sections:
+  - Name:    .dynamic
+    Type:    SHT_DYNAMIC
+    Address: 0x1010
+ProgramHeaders:
+  - Type: PT_LOAD
+    VAddr: 0x1000
+    Sections:
+      - Section: .dynstr
+      - Section: .dynamic
+  - Type: PT_DYNAMIC
+    VAddr: 0x1010
+    Sections:
+      - Section: .dynamic