[PATCH] D129728: [llvm-dwp] Add SHF_COMPRESSED support and remove .zdebug support

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D129728#3653781 <https://reviews.llvm.org/D129728#3653781>, @dblaikie wrote:

> In D129728#3653775 <https://reviews.llvm.org/D129728#3653775>, @MaskRay wrote:
>
>> In D129728#3653769 <https://reviews.llvm.org/D129728#3653769>, @dblaikie wrote:
>>
>>> In D129728#3653748 <https://reviews.llvm.org/D129728#3653748>, @MaskRay wrote:
>>>
>>>> In D129728#3653746 <https://reviews.llvm.org/D129728#3653746>, @dyung wrote:
>>>>
>>>>> Your change is causing the test 'LLVM :: tools/llvm-dwp/X86/nocompress.test' to fail due to one of the expected input files being deleted by your change. Perhaps you need to delete or fixup this test as well?
>>>>>
>>>>> https://lab.llvm.org/buildbot/#/builders/216/builds/7412
>>>>
>>>> I've removed it in 33aa374e597260dae739bb949892cff31ae31eb9 <https://reviews.llvm.org/rG33aa374e597260dae739bb949892cff31ae31eb9> . Someone feeling a need to have a coverage can add it.
>>>
>>> Please don't remove test coverage like this - this test covers the error path here: https://github.com/llvm/llvm-project/blob/33aa374e597260dae739bb949892cff31ae31eb9/llvm/lib/DWP/DWP.cpp#L288
>>
>> It's a fast way to get bots green. It takes time to construct a test.
>
> The usual way to do that if this would take to long is to revert the causal patch while addressing the regression - not lower test coverage permanently.

The file will need to be re-added anyway to avoid the precanned binary. Adding a test in one minute, I am not sure it's useful to revert this current patch.

>> In lld/test/ELF I added tests testing `UNSUPPORTED: zlib`.
>
> Sure - that doesn't test the DWP error handling codepath linked above, though.

This sentence just wanted to say I know that a test is needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129728