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

[Puyan Lotfi via Phabricator via llvm-commits]

plotfi added inline comments.


================
Comment at: test/tools/llvm-objcopy/compress-debug-sections-zlib-gnu.test:32
+# NOTE: Zlib compression makes things worse in this case.
+# SIZE-ORIGINAL: 637
+# SIZE-COMPRESSED: 656
----------------
jhenderson wrote:
> jhenderson wrote:
> > jhenderson wrote:
> > > I think it probably makes more sense to get the section size and compare that, rather than the overall file size. You can drop the "wc" calls that way.
> > Don't do it this way. You can use the section size field directly (available in the sections dump from earlier), rather than extracting the sections.
> > I think it probably makes more sense to get the section size and compare that, rather than the overall file size. You can drop the "wc" calls that way.
> 
> > Don't do it this way. You can use the section size field directly (available in the sections dump from earlier), rather than extracting the sections.
> Why have you marked these as done? They aren't done.
Missed what you meant in your comment. Sorry. 


================
Comment at: tools/llvm-objcopy/Object.h:17
 #include "llvm/BinaryFormat/ELF.h"
+#include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/StringTableBuilder.h"
----------------
alexshap wrote:
> alexshap wrote:
> > jhenderson wrote:
> > > alexshap wrote:
> > > > jhenderson wrote:
> > > > > jhenderson wrote:
> > > > > > plotfi wrote:
> > > > > > > jhenderson wrote:
> > > > > > > > You can get rid of this from the header by forward-declaring the DebugCompressionType enum.
> > > > > > > You can not. Usage of DebugCompressionType::GNU etc makes this not possible unless you want to copy the enum class (which I would like not to).
> > > > > > You aren't using them in the header file, as far as I can see. Move the include into the .cpp if it is needed there. Same goes for Compression.h.
> > > > > You should still forward declare the enum in the header and include MCTargetOptions.h in the cpp file.
> > > > @jhenderson - khm, if we forward declare the enum the size of the enum is unknown (if the definition is not available), consequently, every single .cpp which includes Object.h will have to include MCTargetOptions.h. Just curious - what are the benefits of this / are there any pointers to the llvm style guide or any other docs suggesting this ? that would look kinda broken (one header (Object.h) implicitly depends on another one and can not be included without causing errors into an empty .cpp). 
> > > > if we forward declare the enum the size of the enum is unknown (if the definition is not available), consequently, every single .cpp which includes Object.h will have to include MCTargetOptions.h.
> > > Not true. This is an `enum class` which can be forward declared (defaults to `int` size unless explicitly stated). See https://en.cppreference.com/w/cpp/language/enum. Only clients of the code that actually want to access an enumeration value need to include the header.
> > > 
> > > It's bad practice to include more headers than needed in general, due to the increased compile time cost, and also creation of implicit dependencies that later could result in broken builds.
> > @jhenderson 
> > 
> > 1. Not true.
> > alexshap-mbp:~ alexshap$ cat main.cpp
> > enum F;
> > int main() {
> >   return sizeof(F);
> > }
> > alexshap-mbp:~ alexshap$ clang -std=c++11 main.cpp -o main.exe
> > main.cpp:1:6: error: ISO C++ forbids forward references to 'enum' types
> > enum F;
> >      ^
> > main.cpp:3:10: error: invalid application of 'sizeof' to an incomplete type 'F'
> >   return sizeof(F);
> >          ^     ~~~
> > main.cpp:1:6: note: forward declaration of 'F'
> > enum F;
> >      ^
> > 2 errors generated.
> > 
> > 2. Nobody suggested including more headers than needed,  my point was different,
> > it seems like it would be strange if whenever someone wants to include <vector> and use smth from it (for example, sizeof(std::vector<int>)) he has to include <map> as well.
> > 
> > 
> I see, "enum class" is different, in this case it can be forward declared, I didn't notice that.
> Yeah, in this case it's appropriate and @jhenderson is right.
This will force me to include #include "llvm/MC/MCTargetOptions.h" in both Object.cpp and llvm-objcopy.cpp FYI


Repository:
  rL LLVM

https://reviews.llvm.org/D49678