Page MenuHomePhabricator

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

Authored by kongyi on Sep 30 2019, 3:50 PM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

kongyi created this revision.Sep 30 2019, 3:50 PM
Herald added a project: Restricted Project. · View Herald Transcript
MaskRay added a comment.EditedSep 30 2019, 7:26 PM

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 added inline comments.Oct 1 2019, 1:28 AM
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 added inline comments.Oct 1 2019, 1:39 AM
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)

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.

srhines added a subscriber: srhines.Oct 9 2019, 3:18 PM