[PATCH] D130724: [MC] Support writing ELFCOMPRESS_ZSTD compressed debug info sections

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/MC/ELFObjectWriter.cpp:861-879
+  ArrayRef<uint8_t> Uncompressed =
       makeArrayRef(reinterpret_cast<uint8_t *>(UncompressedData.data()),
-                   UncompressedData.size()),
-      Compressed);
+                   UncompressedData.size());
+
+  SmallVector<uint8_t, 128> Compressed;
+  uint32_t ChType;
+  switch (MAI->compressDebugSections()) {
----------------
MaskRay wrote:
> dblaikie wrote:
> > Proliferation of these sort of switch statements is why I'd like to resolve the design discussions before further patches are committed. They're not hugely expensive to fix/change later, but neither are we in a rush to add this functionality, nor is the design discussion especially hard to resolve.
> > Code something like:
> > ```
> > if (CompressionAlgorithm *C = getCompressionAlgorithm(MAI->compressDebugSections()) {
> >   ChType = C->getELFType(); // alternatively have an array to lookup ELF type from the enum type
> >   C->compress(Uncompressed, Compressed);
> > }
> > ```
> > Seems like it'd be quite achievable without great complexity & simplify consumers.
> > 
> > Possibly even having 'compressDebugSections' carry the algorithm pointer directly? (since the checking's already done up-front, and the pointer would be to an immutable/thread safe/singleton/eternal object anyway)
> I wish that 15.0.0 would have the llvm-objcopy change. This helps distributions which install llvm binary utilities in /usr/bin which may use newer userland llvm utilities.
> MC/clang -gz support certainly can go into 16.0.0.
> 
> After this MC change, the only remaining place using `DebugCompressionType` is clang -gz. We don't have many ELF object producers, and the object-oriented `CompressionAlgorithm` design kinda makes me feel some over-engineering.
> 
> Note: in many (among the few `DebugCompressionType` uses) use cases `isAvailable` and `compress` are called apart.
I don't think there's any need to rush to get this into any particular LLVM release (compression isn't enabled by default in any case & I don't think this is a pressing issue, is it?).

I don't think the complexity needed to implement the sort of framing above should be too great (the current patches add a bunch of other things that I think are overly complicating it & making the comparison weight differently - which is why I'm trying to start with the syntax and work backwards to the implementation necessary to provide that interface and not more than that).

It'd be nice if they didn't have to be so separate (between isAvailable and compress) - so the invariants were expressed at the API level rather than by assertions that something was checked somewhere else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130724