Fix compression header size check in ELF writer #66888
Conversation
The test had 32-bit and 64-bit header sizes the wrong way around.
|
Please add a test case |
|
This test is to see whether compression is worth it. Size <= HdrSize + CompressedContents.size()) Is the original uncompressed size less than or equal to the compressed size plus the compression header? To test the change I'd need to find a case where the DWARF section compressed to approximately the same size as the original data, but the difference in compression header size (12 vs 32 bytes) would have made a difference in the decision taken. That does sound like a fairly tall order. On top of this, I have zero experience with the LLVM test suite. And finally, making a suboptimal compression vs no-compression decision that is at worst 20 bytes wrong isn't going to upset many people. |
|
The code checks whether compressing a section is better than not compressing it. To test this, we need to find a payload where the compressed content is about 12 bytes smaller than the uncompressed content. This is really difficult and would make the test too sensitive to different zlib implementations (we have madler, chromium, zlib-ng, etc and many versions to support). Therefore, I think not having a test in this case is fine. |
|
Could this be unit tested, then? (I realize the class is currently in the .cpp file, so would have to be moved into a header so it'd be accessible from a unit test) |
Likely very brittle to not be dependent on specific zlib versions... Merged as it is better than the wrong status quo.. |
|
If someone wants to add a unittest for this case, it would be highly appreciated. But it's fair to say that the author and existing reviewers don't have bandwidth to do this :) |
That's not generally how testing is handled in the LLVM project. |
|
As it turns out, this breaks at least two lit tests when building with (See https://bugs.chromium.org/p/chromium/issues/detail?id=1499858) Probably the expectations just need updating, and then we'd have this tested :-) I'll revert it for now to unbreak the builds. |
This broke lit tests in zstd enabled builds, see comment on the PR. > The test had 32-bit and 64-bit header sizes the wrong way around. This reverts commit c5ecf5a.
This is #66888 with a test. For MC we only use a zstd test, as zlib has a lot of versions/forks with different speed/size tradeoff, which would make the test more brittle. If clang/test/Misc/cc1as-compress.s turns out to be brittle, we could make the string longer.
This is llvm#66888 with a test. For MC we only use a zstd test, as zlib has a lot of versions/forks with different speed/size tradeoff, which would make the test more brittle. If clang/test/Misc/cc1as-compress.s turns out to be brittle, we could make the string longer.
This is llvm#66888 with a test. For MC we only use a zstd test, as zlib has a lot of versions/forks with different speed/size tradeoff, which would make the test more brittle. If clang/test/Misc/cc1as-compress.s turns out to be brittle, we could make the string longer.
The test had 32-bit and 64-bit header sizes the wrong way around.