[PATCH] D70658: [LLD][ELF] - Make compression level be dependent on -On.

Wed Nov 27 09:32:03 PST 2019

MaskRay added inline comments.

================
Comment at: lld/test/ELF/compressed-debug-level.test:23
+# HEADER: [Nr] Name        Type     Address  Off    Size
+# LEVEL1: [ 1] .debug_info PROGBITS 00000000 000094 00001c
+# LEVEL6: [ 1] .debug_info PROGBITS 00000000 000094 00001b
----------------
smeenai wrote:
> grimar wrote:
> > smeenai wrote:
> > > MaskRay wrote:
> > > > smeenai wrote:
> > > > > The size appears to depend on the zlib version. For example, I'm running with zlib 1.2.8, which manages to compress these contents to the same size with both level 1 and 6. Is there any way to make this agnostic to zlib version?
> > > > I run zlib 1.2.11 and it works for me. Can you alter the input a bit to make the length different for level 1 and level 6, for zlib 1.2.8?
> > > I fixed it with 75fd939bb917e8f843395684a2970d86bc0199c0.
> > > I fixed it with 75fd939bb917e8f843395684a2970d86bc0199c0.
> > 
> > Thanks! FTR, I used zlib 1.2.3 under windows when wrote this. Now I wonder how much brittle to check the size produced.
> > Seems it is not safe enough?
> > I think what we can do instead is to print llvm-readelf output to files for pairs [-O0, -O2], [-O1, -O2] etc and then do something
> > like
> > 
> > ```
> > # CHECK: [ 1] .debug_info PROGBITS 00000000 000094 [[SIZE:*]]
> > # CHECK-NOT: [ 1] .debug_info PROGBITS 00000000 000094 [[SIZE]]
> > ```
> > 
> > i.e. this should check that sized are just different. Should we?
> The issue I ran into with the original test case was that the compressed data had the same size with levels 1 and 6, so just verifying the sizes were different wouldn't help.
> 
> I agree that checking sizes isn't super robust, but it seems to work okay for now with the updated test case? 
We could use `not cmp %t.0 %t.2`, but since it seems to work for zlib 1.2.3, 1.2.8, 1.2.11, probably not worth updating it again.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70658