[PATCH] D49678: [llvm-objcopy] Adding support for compressed DWARF debug sections.

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi marked 3 inline comments as done.
plotfi added inline comments.


================
Comment at: tools/llvm-objcopy/Object.h:269-272
+  virtual const StringRef getContents() const {
+    assert(false && "Section has no Contents.");
+    return "";
+  }
----------------
jhenderson wrote:
> plotfi wrote:
> > jhenderson wrote:
> > > Don't mark return types as const (except for pointers to const and reference returns): the caller can just ignore the const and copy the value, so it has zero use (indeed some compilers actually warn about this). It's also completely redundant for a StringRef, since StringRefs are implicitly const.
> > > 
> > > Should this function just be a pure abstract function if it can't be called?
> > > 
> > > I also don't know whether "getContents" means get original contents or get contents at the end. Please rename more appropriately.
> > What do you mean? I guess the tricky thing here is, it depends on the type of object. I don't see this being useful for anything other than a Section or a OwnedDataSection or a Compressed/Decompressed section. 
> Right, so should this be a part of the base section interface? What you are saying suggests that it shouldn't be.
> 
> Is the purpose of this function to get the original, unmodified section contents, or the contents after they get modified (when the latter is possible)?
Well so, since it seems we are treating these section objects as immutable then it is the unmodified section contents. At the end of the day I need some kind of interface to get the original section content to compress or decompress. 


================
Comment at: tools/llvm-objcopy/Object.h:368
+
+  CompressedSection(std::unique_ptr<SmallVectorImpl<char>> CompressedData)
+      : ModifiedData(std::move(CompressedData)) {}
----------------
jhenderson wrote:
> plotfi wrote:
> > jhenderson wrote:
> > > I'm pretty sure you're supposed to use `SmallVector` directly.
> > But a SmallVector of what size? As far as I know SmallVector always has some size N. In a lot of places when I see SmallVector passed around (as in the case of raw_ostream) it's passed as a SmallVectorImpl&. Should this not be the case here?
> Okay, my bad. I just feel like a class with "impl" in the name implies it's intended for internal use only...!
> 
> But actually, it strikes me that this isn't "Small" anyway, so should probably just be a std::vector directly (that suggests that the compress function interface is incorrect then).
Using SmallVector is just easier for certain things like raw_svector_ostream, I'd prefer to leave this alone. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:354
+std::unique_ptr<SmallVectorImpl<char>>
+decompress(StringRef &Name, StringRef Contents, ElfType OutputElfType) {
+  switch (OutputElfType) {
----------------
jhenderson wrote:
> jakehehrlich wrote:
> > I think you don't need to do this if you create a 'DecompressedSectionType' that knows how to handle write out the decompressed contents because the writer will be able to call the proper decompress<ELFT> method directly!
> @plotfi, you've marked this comment as "Done", yet I don't see you using ELFT directly anywhere. You should be able to use ELFT::TargetEndianness and ELFT::Is64Bits in the library call (although personally, I think that the library interface should take an ELFT, so that these don't need explicitly passing in).
The Decompressor library was not written by be, it doesn't use ELFT. Not going to change Decompressor in this change. 


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:333
+decompress(StringRef Name, StringRef Contents, ElfType OutputElfType) {
+  auto setElfType = [](ElfType OutputElfType) -> std::tuple<bool, bool> {
+    switch (OutputElfType) {
----------------
jhenderson wrote:
> What are you setting here? There is no change in state in this function.
I can change the name. 


Repository:
  rL LLVM

https://reviews.llvm.org/D49678