[PATCH] D79612: [Object] Add tests for parsing invalid .dynamic section.

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon May 11 10:11:53 PDT 2020


MaskRay added a comment.

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...


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79612/new/

https://reviews.llvm.org/D79612





More information about the llvm-commits mailing list