Page MenuHomePhabricator

[Driver] Remove obsoleted -gz=zlib-gnu
ClosedPublic

Authored by MaskRay on Jan 19 2022, 9:02 PM.

Details

Summary

GCC added -gz=zlib-gnu in 2014 for -gz meaning change (.zdebug =>
SHF_COMPRESSED) and the legacy zlib-gnu hasn't gain adoption.

According to Debian Code Search, no project uses -gz=zlib-gnu (valgrind has a
configure to use -gz=zlib). Any possible -gz=zlib-gnu user can switch to -gz
smoothly (supported by integrated assemblers for many years; binutils 2.26).

Diff Detail

Event Timeline

MaskRay created this revision.Jan 19 2022, 9:02 PM
MaskRay requested review of this revision.Jan 19 2022, 9:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2022, 9:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MaskRay updated this revision to Diff 401498.Jan 19 2022, 9:16 PM

fix a comment

According to Debian Code Search, no project uses -gz=zlib-gnu (valgrind has a configure to use -gz=zlib)

Could you link to the queries you used to validate this? I'd be curious to take a look around.

According to Debian Code Search, no project uses -gz=zlib-gnu (valgrind has a configure to use -gz=zlib)

Could you link to the queries you used to validate this? I'd be curious to take a look around.

https://codesearch.debian.net/search?q=gz%3Dzlib-gnu&literal=0

dblaikie accepted this revision.Jan 26 2022, 11:50 AM

I don't especially feel that removing low-cost features like this is a great idea (the benefit to cleaning up a small amount of code compared to the (albeit small) risk of breaking users doesn't seem high value to me), but sure - give it a go.

This revision is now accepted and ready to land.Jan 26 2022, 11:50 AM
MaskRay added a comment.EditedJan 26 2022, 12:02 PM

I don't especially feel that removing low-cost features like this is a great idea (the benefit to cleaning up a small amount of code compared to the (albeit small) risk of breaking users doesn't seem high value to me), but sure - give it a go.

Thanks:)

This is to ensure that there are no producers: then we can do something more ambitious, like removing assembler support:)

Driver change is a good start because if someone has justifiable complaint, the change can be relatively easily reversed without churn to the underlying implementation.

It will also make future removal of .zdebug from ld.lld justifiable.

I don't especially feel that removing low-cost features like this is a great idea (the benefit to cleaning up a small amount of code compared to the (albeit small) risk of breaking users doesn't seem high value to me), but sure - give it a go.

Thanks:)

This is to ensure that there are no producers: then we can do something more ambitious, like removing assembler support:)

Driver change is a good start because if someone has justifiable complaint, the change can be relatively easily reversed without churn to the underlying implementation.

It will also make future removal of .zdebug from ld.lld justifiable.

Yeah, I appreciate the desire, but it doesn't seem especially burdensome to support (since we'll still have support for the SHF_COMPRESSED style and it's not a lot of code on top of that to support the zdebug style) so I don't see as much value in making these changes, but that's OK.

Oh, one thing: would probably be good to make two separate commits, one for -gz and one for the --compress-debug-sections

I don't especially feel that removing low-cost features like this is a great idea (the benefit to cleaning up a small amount of code compared to the (albeit small) risk of breaking users doesn't seem high value to me), but sure - give it a go.

Thanks:)

This is to ensure that there are no producers: then we can do something more ambitious, like removing assembler support:)

Driver change is a good start because if someone has justifiable complaint, the change can be relatively easily reversed without churn to the underlying implementation.

It will also make future removal of .zdebug from ld.lld justifiable.

Yeah, I appreciate the desire, but it doesn't seem especially burdensome to support (since we'll still have support for the SHF_COMPRESSED style and it's not a lot of code on top of that to support the zdebug style) so I don't see as much value in making these changes, but that's OK.

The issue with .zdebug is the section name. When iterating over input sections (which may cost ld.lld 0.4% time, the ratio will go up if we support parallel symbol table initialization), not needing to scan section names (different places from the section metadata) will improve a bit.
Yeah, it's saving on some very minor stuff, I know.

Oh, one thing: would probably be good to make two separate commits, one for -gz and one for the --compress-debug-sections

OK

labath added a subscriber: labath.Jan 27 2022, 1:26 AM

This (unsurprisingly) broke an lldb test for the gnu style compression. You didn't get the notification because the bot was already red at the time. I've already fixed the situation by converting the test to yaml, but I thought you may want to know what happened.

MaskRay added a comment.EditedJan 27 2022, 12:45 PM

This (unsurprisingly) broke an lldb test for the gnu style compression. You didn't get the notification because the bot was already red at the time. I've already fixed the situation by converting the test to yaml, but I thought you may want to know what happened.

Thanks for aaa9f40e3fd2dd081e965d39f10649bcd8bf1d21 :) Sorry that I missed -Wa,--compress-debug-sections=zlib-gnu in the test.

To be accurate, the zlib-gnu style compression is still available with llvm-mc -filetype=obj -compress-debug-sections=zlib-gnu (that I haven't dropped but will do after a while).
It's just that the producer feature is not expoed in any user-facing tool (clang, lld).

And sound good to remove lldb reading support at some point.
I have found that Go's own linker generates .zdebug and notified them to fix it (https://go-review.googlesource.com/c/go/+/380755/)
If lldb debugability with older Go (once they have fixed the issue) is not so useful, yeah, drop the support.