objcopy --only-keep-debug flag creates files with empty dynamic section. llvm-objdump should not throw error for these files.
I would argue that this is indeed an invalid case, though objdump -p does not error.
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?
We can move this test into elf-dynamic-section.test or rename it to elf-dynamic-section-*.test.
For --only-keep-debug, this is SHT_NOBITS
DT_NULL is a mandatory field according to the spec. I think it is invalid to remove if (Dyn.empty()) check here.
Perhaps, you should add a flag like bool AllowEmpty or do a change on a llvm-objdump side instead.
I have an error when trying to use this YAML as is:
(I've added a dummy .dynstr declaration to procceed)
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
- Search for PT_DYNAMIC segment. Find it.
- Load data from the segment. Resultant Dyn is empty (because p_filesz is 0).
- Dyn is empty, so attempt to load data from section.
- No SHT_DYNAMIC section is found, so Dyn remains empty.
- 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.
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.
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.