[PATCH] D51841: [llvm-objcopy] Dwarf decompression support.

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi added inline comments.


================
Comment at: tools/llvm-objcopy/Object.h:274
   uint64_t Type = ELF::SHT_NULL;
+  uint64_t DecompressedSize = 0;
+  uint64_t DecompressedAlign = 1;
----------------
jakehehrlich wrote:
> Why do these need to be moved into SectionBase?
Because I set them on "Sec" after makeSection is called. I think maybe if we reuse CompressedSection or add a DecompressedSection this could be moved elsewhere. 


================
Comment at: tools/llvm-objcopy/Object.h:366
+      : SectionBase(Sec), DoDecompression(DoDecompression) {
+    if (!DoDecompression)
+      return;
----------------
jakehehrlich wrote:
> alexshap wrote:
> > alexshap wrote:
> > > alexshap wrote:
> > > > I'm looking at the changes in OwnedDataSection, tbh, i'm not a big fan of them.
> > > > It looks like OwnedDataSection is not the right place for them (now we have a bool flag and a new branch everywhere + some details (like ".zdebug") have leaked into it).  
> > > one potential approach might be to create DecompressedSection class. cc: @jakehehrlich
> > > (to avoid misunderstanding: ubiquitous "if(DoCompression)" or "if(Sec.DoCompression)" all over the pipeline suggest that this dispatching mechanism should not be in OwnedDataSection)
> > typo: DoDecompression
> +1 on this general view point. OwnedDataSection is not the right place.
Sounds good. @jakehehrlich Would you prefer I reuse CompressedSection or write a new DecompressedSection class? I'll send out a new patch after you let me know. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:426
+static void
+replaceSections(const CopyConfig &Config, Object &Obj, SectionPred &RemovePred,
+                std::function<bool(const SectionBase &)> doReplacement,
----------------
jakehehrlich wrote:
> So section replacement is very tricky to do right. It's one of the fundamental operations (like remove and add) that I want on Object. I have not implemented it yet because, well, it's complicated. It's easy to do when no relocations, symbols, or other parts of the system reference the given section but we're almost never so lucky. In the previous change we dealt with relocations via this method but using a different, less generic name. It might be worth doing that again. So either the name should change, or we should bite the bullet and implement a proper replace method in Object. Implementing a proper replace method will require a wide scale change to llvm-objcopy but would have a lot of benefits. My thinking was that replace could be implemented by adding a new method to SectionBase called "replaceSection" much like "removeSection" works now. Architecturally that's pretty simple. The problem is that *every* section needs to be extended with this method and every detail has to be implemented correctly. 
> 
> If you wanna do it, I'd be super happy to review it. If not, we can just rename this function to something less generic for now. Maybe something like "replaceDebugSections" since it really only handles relocations.
Yeah would be a good thing to think about. Alternatively I could maybe decompress the section into a "DecompressedSection" on read, and instead of replacing the decompressed sections I could possibly just flag them for decompressed write? I agree, better to not encourage people to reuse this outside of the context of debug sections. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:676
+                      return &Obj.addSection<OwnedDataSection>(
+                          S, true /* DoDecompression */);
+                    });
----------------
jakehehrlich wrote:
> Instead of modifying OwnedDataSection can you just do the decompression here?
Sure. Would you prefer I reuse CompressedSection somehow (adding some decompression flag) or make a whole new DecompressedSection class?


Repository:
  rL LLVM

https://reviews.llvm.org/D51841