[Object] Add tests for parsing invalid .dynamic section.

[jhenderson]

The test looks good, but I'm thinking about the location. Should it be in llvm-objdump's test directory? What about testing llvm-readobj - how does it handle malformed dynamic sections?

Should this test actually be a couple of gtest unittests in unittest/Object?

@MaskRay/@grimar, do either of you have any thoughts?

[MaskRay]

This will duplicate a lot of .dynamic .dynsym related tests in test/tools/llvm-readobj. We have several choices:

1. Duplicate the tests in test/tools/llvm-objdump: which means improvement to test/tools/llvm-readobj will not be reflected to llvm-objdump
2. Move test/tools/llvm-readobj tests to test/Object, so that conceptually they are easier to be acceptable that llvm-objdump also uses them. Maybe some parsing logic should be moved to lib/Object. Then, unittests/Object
3. Add llvm-objdump tests to test/tools/llvm-readobj. We can add a subdirectory test/tools/llvm-readobj/ELF/llvm-objdump/ to make such tests separate from the rest llvm-readobj tests.

Currently I am thinking 2 will not share much code, and we may end up with harder to refactor code. 1 has the problem of cross-directory references: a code change will have non-local effect. This oftentimes happens with the rest of LLVM, but thankfully for many LLVM binary utilities we have a nice property: updating a tool's code oftentimes just affects test/Object and test/tools/llvm-foo.
In the end, I feel that 3 is not that unacceptable...

[grimar]

We have an existent test for testing similar cases: llvm\test\Object\invalid.test. I'd definitely try to avoid adding one more elf-invalid-* test case to llvm\test\Object.
I think we might want to make a cleanup and split/refine this test, but probably not necessary right now.
(I believe that having a unittest for each error lib/Object reports would be the best possible way to go. Because API tests ideally should be independent from tools tests,
as tools might change the behavior and break the coverage.)

This patch could just add test cases to llvm-objdump's test directory. And so its title could be something like: [llvm-objdump] - Test dumping of invalid .dynamic sections".
Testing the llvm-readobj (assuming we have no similar tests) can be a follow-up independent change and seems useful to me as might produce a different output for the same cases.

[jhenderson]

I've been sitting on this for a few days, and here's what I think would be my ideal situation:

1. Library code paths should be comprehensively tested via unit tests.
2. Tools using the library should just test that errors returned via the library interface are handled appropriately. If you treat the library as a black box, that just means each library call only needs a single corresponding test case in each tool that calls into it, to show how the Error/Expected return value is handled. Such lit tests would exist in the corresponding tool folder. They would inevitably slightly overlap with library unit tests, but their focus isn't the same (and appropriate test naming/commenting can probably illustrate that).

Additional test cases might need to exist at a tool level to show that a specific behaviour in the tool circumvents something that might otherwise fire from a later library call (e.g. say a tool had special handling for missing DT_NULL entries), but those tests would be testing the circumvention code path rather than the error handling. If somebody changes the behaviour of a tool to add special handling on that, they should of course ensure that the new behaviour is tested in the tool.

That's not the situation we're in at the moment, but I think we can at least ensure new tests follow that style, by preferring unit tests for the library changes (or in this case, untested library paths). If somebody wants to start porting lit tests to unittests, I'm certainly not going to stop them though.

So, my conclusion is that this new lit test shouldn't exist, but rather that the two test cases should be two unit tests in the Object library. The only issue might be in setting something like that up, but I know that, e.g. the Minidump tests use YAML as a basis to generate an object, so you might be able to follow them to create a malformed dynamic section without too much difficulty.

llvm/lib/Object/ELF.cpp

for (const Elf_Shdr &Sec : *SectionsOrError) {
       }
     }
 
     if (!Dyn.data())
       return ArrayRef<Elf_Dyn>();
   }
 
   if (Dyn.empty())
-    // TODO: this error is untested.
     return createError("invalid empty 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();

llvm/test/Object/elf-invalid-dynamic-section.test

+## Test LLVM emits a proper error message when parsing an invalid .dynamic section.
+
+# RUN: yaml2obj --docnum=1 %s -o %t1.o
+# RUN: not llvm-objdump -p %t1.o 2>&1 | FileCheck %s --check-prefix=EMPTY
+
+# EMPTY: error: '{{.*}}': invalid empty dynamic section
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name:    .dynamic
+    Type:    SHT_DYNAMIC
+    Content: ""
+
+# RUN: yaml2obj --docnum=2 %s -o %t2.o
+# RUN: not llvm-objdump -p %t2.o 2>&1 | FileCheck %s --check-prefix=MALFORMED
+
+# MALFORMED: error: '{{.*}}': dynamic sections must be DT_NULL terminated
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_DYN
+  Machine: EM_X86_64
+Sections:
+  - Name: .dynamic
+    Type: SHT_DYNAMIC
+    Entries:
+      - Tag:   DT_NEEDED
+        Value: 0x1