[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#3653821 <https://reviews.llvm.org/D129728#3653821>, @dblaikie wrote:

> In D129728#3653818 <https://reviews.llvm.org/D129728#3653818>, @MaskRay wrote:
>
>> 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.
>
> Ah, OK - thanks! Nah, I don't mind fixing forward here, though I think in the future it'd be simpler/clearer if the patch were reverted and recommitted correctly rather than temporarily removing testing like this.

This patch (simple enough but still touched quite a few files), maybe. But I realized that such a "failing-to-testing-a-case-people-don't-tend-to-use" may not be sufficient for some other tests.
The net result fixing forward here isn't too bad. The history is quite clean, just that the `nocompress.test` file has some 1 unneeded log.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129728