[PATCH] D134385: [llvm-objcopy] --compress-debug-sections: remove tail padding for ELFCLASS32

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked an inline comment as done.
MaskRay added inline comments.


================
Comment at: llvm/lib/ObjCopy/ELF/ELFObject.cpp:538-539
   Flags |= ELF::SHF_COMPRESSED;
-  size_t ChdrSize =
-      std::max(std::max(sizeof(object::Elf_Chdr_Impl<object::ELF64LE>),
-                        sizeof(object::Elf_Chdr_Impl<object::ELF64BE>)),
-               std::max(sizeof(object::Elf_Chdr_Impl<object::ELF32LE>),
-                        sizeof(object::Elf_Chdr_Impl<object::ELF32BE>)));
+  size_t ChdrSize = Is64Bits ? sizeof(object::Elf_Chdr_Impl<object::ELF64LE>)
+                             : sizeof(object::Elf_Chdr_Impl<object::ELF32LE>);
   Size = ChdrSize + CompressedData.size();
----------------
jhenderson wrote:
> Actually one question: this seems to be assuming that BE and LE Elf_Chdr_Impl are the same size. I assume that's intentional, and it's fine if it is, but perhaps it deserves a `static_assert` or similar to protect us in case something odd changes in the future?
Thanks for the quick review. ELF has ELFCLASS32 and ELFCLASS64 distinction and endianness does not affect the structures. LLVMObject defines `Elf_Chdr_Impl` to provide ergonomic access for cross-endianness situations but I feel static_assert is rather unnecessary here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134385