[PATCH] D64800: [llvm-readobj] - Stop using precompiled objects in file-headers.test

Thu Jul 18 01:45:16 PDT 2019

jhenderson added a comment.

In D64800#1589843 <https://reviews.llvm.org/D64800#1589843>, @grimar wrote:

> In D64800#1589215 <https://reviews.llvm.org/D64800#1589215>, @jhenderson wrote:
>
> > I feel look we've lost some test coverage here: a number of the checked fields have gone from being non-zero to being zero. This means that we're no longer testing that the property is actually read and printed correctly, so I think the YAML needs updating in these cases.
>
>
> Let me clarify. Do you mean, for example, that for `COFF-ARM64`, `SectionCount` became `0`, but was non-zero before?

Yes, that's exactly what I was talking about.

> (Btw, my motivation of doing this patch was: I want to simplify another test by moving its parts to another places,
>  like this one and eliminate. Generally I found very usefull to have no binaries in the any inputs, removing them should allow us to significantly
>  cleanup and improve the existent tests and also the tools).

No problem with the motivation at all. Having canned inputs can cause headaches too.

I think part of the problem is that there are WAY too many test cases in this single test file. I think it would be much simpler if we split it up into e.g. file-headers-coff.test, file-headers-elf.test etc (or similar). Further, I think they could be further divided to separate out the machine type testing from testing the other fields (i.e. having a test for file-headers-elf-machine.test or similar, which tests just the Format and Arch values). This would be a much smaller test. Only when an architecture has special behaviour for certain fields should we consider testing it more in detail. Each of the fields could then be tested individually in this way (or to a limited extent at least). It would lead to many more tests, but each individual test case would be much smaller, as it only needs to check one small thing.

I do think there is value in both 0 and non-zero values for most fields. These would become much simpler if the test cases were split up as I've described. Thus there could be elf-file-headers-section-count.s, elf-file-headers-shentsize.s, elf-file-headers-shstrndx.s, coff-file-headers-section-count.s etc.

Does that make sense? I know it's more work, and perhaps this is an okay change as an interim level, as long as we're moving towards the suggestion, and long-term don't lose test coverage.

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

https://reviews.llvm.org/D64800