llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.25k stars 11.67k forks source link

Rename llvm::DebugCompressionType #87027

Open MaskRay opened 6 months ago

MaskRay commented 6 months ago

llvm::DebugCompressionType was added to support ELF compression, used by --compress-debug-sections in assembler/linker/llvm-objcopy/etc. The type name made lots of sense in the past when the use cases were specific to .debug_* sections. The name now makes less sense as llvm-readelf -z -x, ld.lld --compress-sections (and llvm-objcopy --compress-sections) can (de)compress arbitrary sections.

We should probably rename llvm::DebugCompressionType. There are a few choices:

We also have llvm::compression::Format, which can be used by other object file formats if they ever want to support compression. llvm::compression::Format does not have a "no-compression" value while llvm::DebugCompressionType has None.

llvmbot commented 6 months ago

@llvm/issue-subscribers-lld-elf

Author: Fangrui Song (MaskRay)

`llvm::DebugCompressionType` was added to support gas/objcopy style `--compress-debug-sections`. The type name makes less sense now that `llvm-readelf -z -x` and `ld.lld --compress-sections` can (de)compress arbitrary sections. We should probably migrate the uses cases `llvm::DebugCompressionType`, but we have `DebugCompressionType CompressionType` variable names that should be changed to avoid same type/variable name.
MaskRay commented 6 months ago

@compnerd @dwblaikie @jh7370 @PiJoules @petrhosek

compnerd commented 6 months ago

Could we not add the none type to llvm::compression::Format and just have a single enumeration? We have too many cases of duplicates, let's try to avoid them if possible.

dwblaikie commented 6 months ago

I was going to suggest that llvm::DebugCompressionType was 1:1 with the SHF_COMPRESSED kinds, but it seems that isn't the case - well, at least it isn't /depended/ upon. eg: https://github.com/llvm/llvm-project/blob/3a106e5b2cd9f4073b2961b991ebaeee96786309/llvm/lib/Object/Decompressor.cpp#L42-L51

So, if it's not 1:1, so long as we have an enum that is at least as descriptive as the ELF compression kinds we support (if someone added an ELF compression kind we wouldn't/didn't support for some time - a switch like the one above could bail out, it wouldn't need to translate it into this underlying enum/representation) - there doesn't seem to be a great need to have more than one enum?

jh7370 commented 6 months ago

Could we not add the none type to llvm::compression::Format and just have a single enumeration? We have too many cases of duplicates, let's try to avoid them if possible.

+1 to this (or something equivalent): we have two distinct enums in Compression.h that do the same thing, except for the "none" value. I can't see any reason why we don't just have a single one: any code paths that use the Format enum that don't want to support a none value can just have an assert/llvm_unreachable added as appropriate.