This is an archive of the discontinued LLVM Phabricator instance.

[llvm-dwp] Add SHF_COMPRESSED support and remove .zdebug support
ClosedPublic

Authored by MaskRay on Jul 13 2022, 9:12 PM.

Details

Summary

clang 14 removed -gz=zlib-gnu and ld.lld/llvm-objcopy removed .zdebug support
recently. llvm-dwp currently doesn't support SHF_COMPRESSED. Add support and
remove .zdebug support.

Simplify llvm::object::Decompressor which has no .zdebug user now.

While here, add tests for ELF32LE, ELF32BE, and ELF64BE.

Diff Detail

Event Timeline

MaskRay created this revision.Jul 13 2022, 9:12 PM
MaskRay requested review of this revision.Jul 13 2022, 9:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 9:13 PM
dblaikie accepted this revision.Jul 14 2022, 3:53 PM

Looks OK, thanks!

llvm/lib/DWP/DWP.cpp
282–287

This should probably be in a separate patch with test coverage (though generally I don't think adding functionality/doing work on llvm-dwp is really worth it - it probably needs to be thrown out & start again to make it sufficiently efficient in terms of memory usage, etc - though with the WebAssembly (so the costly abstractions are now load-bearing to support WebAssembly) support added & lld's non-generality (so doesn't have sufficient abstractions to make it trivial to implement over different formats), it's not clear what the foundation of such a new implementation would be, whether the generality can be maintained while achieving the desired performance, etc).

A FIXME could document what's entailed in adding the endian/size functionality here.

This revision is now accepted and ready to land.Jul 14 2022, 3:53 PM
MaskRay added inline comments.
llvm/lib/DWP/DWP.cpp
282–287

Will add a TODO that this is untested (some binary utilities use TODO to indicate no test coverage as well).

Thanks for the insight that llvm-dwp probably should be re-implemented. I haven't followed the development...

(@ckissane)

dblaikie added inline comments.Jul 14 2022, 4:01 PM
llvm/lib/DWP/DWP.cpp
282–287

I'd prefer it not be checked in untested.

dblaikie added inline comments.Jul 14 2022, 4:05 PM
llvm/lib/DWP/DWP.cpp
282–287

Sorry, should've provided more context: There are some cases where I think untested functionality is warranted, when testing is particularly difficult & manual testing is done as a stop-gap.

In this case I don't think it's much of a value-add, the functionality isn't used much & sort of a paradox of "if it's used just enough to be useful/but then untested, that's especially unfortunate".

If there's concern about this misbehaving, I'd be open to an assertion with a FIXME about how to support other things if someone comes across the assertion.

MaskRay updated this revision to Diff 444829.Jul 14 2022, 4:13 PM

Test {32,64} x {LSB,MSB}

MaskRay marked 2 inline comments as done.Jul 14 2022, 4:14 PM
MaskRay added inline comments.
llvm/lib/DWP/DWP.cpp
282–287

It's actually not too difficult to test all 4 ELF types. Just added the tests.

dblaikie accepted this revision.Jul 14 2022, 4:16 PM

Sounds good :)

llvm/test/tools/llvm-dwp/X86/compress.test
35–36

Might be more readable to not use a default (=64) so the RUN lines are explicit about 32/64 in the same way they are explicit about LSB/MSB?

MaskRay marked 2 inline comments as done.Jul 14 2022, 4:18 PM
MaskRay added inline comments.
llvm/test/tools/llvm-dwp/X86/compress.test
35–36

Thanks for the suggestion. Will push with the change.

MaskRay edited the summary of this revision. (Show Details)Jul 14 2022, 4:19 PM
This revision was landed with ongoing or failed builds.Jul 14 2022, 4:19 PM
This revision was automatically updated to reflect the committed changes.
MaskRay marked an inline comment as done.
dyung added a subscriber: dyung.Jul 14 2022, 5:22 PM

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

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 . Someone feeling a need to have a coverage can add it.

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

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 . Someone feeling a need to have a coverage can add it.

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 . 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. In lld/test/ELF I added tests testing UNSUPPORTED: zlib.

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

In lld/test/ELF I added tests testing UNSUPPORTED: zlib.

Sure - that doesn't test the DWP error handling codepath linked above, though.

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

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

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