[PATCH] elfcompress: Don't rewrite file if no section data needs to be updated.

[PATCH] elfcompress: Don't rewrite file if no section data needs to be updated.

[Mark Wielaard]

If the input and output file are the same and no section needs to
be updated we really don't need to rewrite the file. Check whether
any matching section is already compressed or (GNU) decompressed.
Skip the section if it doesn't need to be changed. If no section data
needs updating end with success without rewriting/updating file.
With --force the file will still always be updated/rewritten even if
no section data needs to be (de)compressed.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 src/ChangeLog     |  6 ++++++
 src/elfcompress.c | 43 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 0e9ab301..50a7045a 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,9 @@
+2018-07-21  Mark Wielaard  <mark@klomp.org>
+
+	* elfcompress.c (get_sections): New function.
+	(process_file): Check whether section needs to change. Don't rewrite
+	file if no section data needs changing.
+
 2018-07-21  Mark Wielaard  <mark@klomp.org>
 
 	* elfcompress.c (process_file): Swap fchmod and fchown calls.
diff --git a/src/elfcompress.c b/src/elfcompress.c
index 1a0f9845..ae4708c5 100644
--- a/src/elfcompress.c
+++ b/src/elfcompress.c
@@ -1,5 +1,5 @@
 /* Compress or decompress an ELF file.
-   Copyright (C) 2015, 2016 Red Hat, Inc.
+   Copyright (C) 2015, 2016, 2018 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -286,6 +286,15 @@ process_file (const char *fname)
     return (sections[ndx / WORD_BITS] & (1U << (ndx % WORD_BITS))) != 0;
   }
 
+  /* How many sections are we going to change?  */
+  size_t get_sections (void)
+  {
+    size_t s = 0;
+    for (size_t i = 0; i < shnum / WORD_BITS + 1; i++)
+      s += __builtin_popcount (sections[i]);
+    return s;
+  }
+
   int cleanup (int res)
   {
     elf_end (elf);
@@ -422,6 +431,9 @@ process_file (const char *fname)
      names change and whether there is a symbol table that might need
      to be adjusted be if the section header name table is changed.
 
+     If nothing needs changing, and the input and output file are the
+     same, we are done.
+
      Second a collection pass that creates the Elf sections and copies
      the data.  This pass will compress/decompress section data when
      needed.  And it will collect all data needed if we'll need to
@@ -464,7 +476,26 @@ process_file (const char *fname)
 
       if (section_name_matches (sname))
 	{
-	  if (shdr->sh_type != SHT_NOBITS
+	  if (!force && type == T_DECOMPRESS
+	      && (shdr->sh_flags & SHF_COMPRESSED) == 0
+	      && strncmp (sname, ".zdebug", strlen (".zdebug")) != 0)
+	    {
+	      if (verbose > 0)
+		printf ("[%zd] %s already decompressed\n", ndx, sname);
+	    }
+	  else if (!force && type == T_COMPRESS_ZLIB
+		   && (shdr->sh_flags & SHF_COMPRESSED) != 0)
+	    {
+	      if (verbose > 0)
+		printf ("[%zd] %s already compressed\n", ndx, sname);
+	    }
+	  else if (!force && type == T_COMPRESS_GNU
+		   && strncmp (sname, ".zdebug", strlen (".zdebug")) == 0)
+	    {
+	      if (verbose > 0)
+		printf ("[%zd] %s already GNU compressed\n", ndx, sname);
+	    }
+	  else if (shdr->sh_type != SHT_NOBITS
 	      && (shdr->sh_flags & SHF_ALLOC) == 0)
 	    {
 	      set_section (ndx);
@@ -518,6 +549,14 @@ process_file (const char *fname)
 	  }
     }
 
+  if (foutput == NULL && get_sections () == 0)
+    {
+      if (verbose > 0)
+	printf ("Nothing to do.\n");
+      fnew = NULL;
+      return cleanup (0);
+    }
+
   if (adjust_names)
     {
       names = dwelf_strtab_init (true);
-- 
2.18.0

Re: [PATCH] elfcompress: Don't rewrite file if no section data needs to be updated.

[Mark Wielaard]

[-- Attachment #1: Type: text/plain, Size: 1590 bytes --]

Hi Igor,

On Sun, Jul 22, 2018 at 10:26:19AM +0200, Igor Gnatenko wrote:
> > With --force the file will still always be updated/rewritten even if
> > no section data needs to be (de)compressed.
> 
> I have doubts about behavior in last sentence. In --help, it's written
> "Force compression of section even if it would become larger". So usually
> people would use this option just to make sure that it would really
> compress/decompress sections.

It is only used for compression. You can use --force to compress
sections even if that makes the ELF section data larger. This is
arguably useless, so only really used for testing purposes.

> However, if nothing needs
> compression/decompression - why to rewrite file?

Indeed. That is why I wrote this patch. Because it might still change
the file. It might remove some padding between sections, correct
alignment of some data in the file, change permission/ownership, etc.

In general you wouldn't want that if not necessary for (de)compression
of some ELF section data.

Again that is arguably useless, so only really used for testing purposes
to see that elfcompress works correctly.

> Probably we should create
> new option --force-on-noop or -f -f or something like that?

If possible I would like only one option for "do something useless even
if not necessary".

The updated attached patch does update the help text:

  -f, --force          Force compression of section even if it would
                       become larger or update/rewrite the file even if
                       no section would be (de)compressed
Cheers,

Mark

[-- Attachment #2: 0001-elfcompress-Don-t-rewrite-file-if-no-section-data-ne.patch --]
[-- Type: text/x-diff, Size: 4378 bytes --]

From d906260e480c4566da9173b50ee1f4fc229f4102 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Sat, 21 Jul 2018 17:07:12 +0200
Subject: [PATCH] elfcompress: Don't rewrite file if no section data needs to
 be updated.

If the input and output file are the same and no section needs to
be updated we really don't need to rewrite the file.

Check whether any matching section is already compressed or decompressed.
Skip the section if it doesn't need to be changed. If no section data
needs updating end with success without rewriting/updating file.

With --force the file will still always be updated/rewritten even if
no section data needs to be (de)compressed.

Signed-off-by: Mark Wielaard <mark@klomp.org>
---
 src/ChangeLog     |  7 +++++++
 src/elfcompress.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 0e9ab301..39214c39 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,10 @@
+2018-07-21  Mark Wielaard  <mark@klomp.org>
+
+	* elfcompress.c (get_sections): New function.
+	(process_file): Check whether section needs to change. Don't rewrite
+	file if no section data needs changing.
+	(main): Update 'force' help text.
+
 2018-07-21  Mark Wielaard  <mark@klomp.org>
 
 	* elfcompress.c (process_file): Swap fchmod and fchown calls.
diff --git a/src/elfcompress.c b/src/elfcompress.c
index 1a0f9845..6ba6af41 100644
--- a/src/elfcompress.c
+++ b/src/elfcompress.c
@@ -1,5 +1,5 @@
 /* Compress or decompress an ELF file.
-   Copyright (C) 2015, 2016 Red Hat, Inc.
+   Copyright (C) 2015, 2016, 2018 Red Hat, Inc.
    This file is part of elfutils.
 
    This file is free software; you can redistribute it and/or modify
@@ -286,6 +286,15 @@ process_file (const char *fname)
     return (sections[ndx / WORD_BITS] & (1U << (ndx % WORD_BITS))) != 0;
   }
 
+  /* How many sections are we going to change?  */
+  size_t get_sections (void)
+  {
+    size_t s = 0;
+    for (size_t i = 0; i < shnum / WORD_BITS + 1; i++)
+      s += __builtin_popcount (sections[i]);
+    return s;
+  }
+
   int cleanup (int res)
   {
     elf_end (elf);
@@ -422,6 +431,9 @@ process_file (const char *fname)
      names change and whether there is a symbol table that might need
      to be adjusted be if the section header name table is changed.
 
+     If nothing needs changing, and the input and output file are the
+     same, we are done.
+
      Second a collection pass that creates the Elf sections and copies
      the data.  This pass will compress/decompress section data when
      needed.  And it will collect all data needed if we'll need to
@@ -464,7 +476,26 @@ process_file (const char *fname)
 
       if (section_name_matches (sname))
 	{
-	  if (shdr->sh_type != SHT_NOBITS
+	  if (!force && type == T_DECOMPRESS
+	      && (shdr->sh_flags & SHF_COMPRESSED) == 0
+	      && strncmp (sname, ".zdebug", strlen (".zdebug")) != 0)
+	    {
+	      if (verbose > 0)
+		printf ("[%zd] %s already decompressed\n", ndx, sname);
+	    }
+	  else if (!force && type == T_COMPRESS_ZLIB
+		   && (shdr->sh_flags & SHF_COMPRESSED) != 0)
+	    {
+	      if (verbose > 0)
+		printf ("[%zd] %s already compressed\n", ndx, sname);
+	    }
+	  else if (!force && type == T_COMPRESS_GNU
+		   && strncmp (sname, ".zdebug", strlen (".zdebug")) == 0)
+	    {
+	      if (verbose > 0)
+		printf ("[%zd] %s already GNU compressed\n", ndx, sname);
+	    }
+	  else if (shdr->sh_type != SHT_NOBITS
 	      && (shdr->sh_flags & SHF_ALLOC) == 0)
 	    {
 	      set_section (ndx);
@@ -518,6 +549,14 @@ process_file (const char *fname)
 	  }
     }
 
+  if (foutput == NULL && get_sections () == 0)
+    {
+      if (verbose > 0)
+	printf ("Nothing to do.\n");
+      fnew = NULL;
+      return cleanup (0);
+    }
+
   if (adjust_names)
     {
       names = dwelf_strtab_init (true);
@@ -1279,7 +1318,7 @@ main (int argc, char **argv)
 	N_("Print a message for each section being (de)compressed"),
 	0 },
       { "force", 'f', NULL, 0,
-	N_("Force compression of section even if it would become larger"),
+	N_("Force compression of section even if it would become larger or update/rewrite the file even if no section would be (de)compressed"),
 	0 },
       { "permissive", 'p', NULL, 0,
 	N_("Relax a few rules to handle slightly broken ELF files"),
-- 
2.18.0

Re: [PATCH] elfcompress: Don't rewrite file if no section data needs to be updated.

[Mark Wielaard]

Hi Igor,

On Sun, Jul 22, 2018 at 10:06:47PM +0200, Igor Gnatenko wrote:
> On Sun, Jul 22, 2018 at 5:20 PM Mark Wielaard <mark@klomp.org> wrote:
> > > However, if nothing needs
> > > compression/decompression - why to rewrite file?
> >
> > Indeed. That is why I wrote this patch. Because it might still change
> > the file. It might remove some padding between sections, correct
> > alignment of some data in the file, change permission/ownership, etc.
> >
> > In general you wouldn't want that if not necessary for (de)compression
> > of some ELF section data.
> 
> But we want this in RPM, right?

Sorry, I don't understand what 'this' refers to.
What do you want in RPM?
I think you would never want to use --force for RPM.

> > Again that is arguably useless, so only really used for testing purposes
> > to see that elfcompress works correctly.
> >
> > > Probably we should create
> > > new option --force-on-noop or -f -f or something like that?
> >
> > If possible I would like only one option for "do something useless even
> > if not necessary".
> 
> Can we just make it to not rewrite file if it won't be changed at all?

I believe that is what this patch does. But I might misunderstand your
question.

Before this patch elfcompress would always rewrite/update the file even
if nothing needed to be (de)compressed. With this patch it will not
change the file if it detects nothing needs to be done.

Unless you use --force to say you really want the file to be updated
anyway even if it was already complete (de)compressed.

The idea behind the patch is that you might want to run eu-elfcompress
on everything to make sure that everything has compressed (debug) ELF
sections (or run eu-elfcompress -t none to make sure no file has). But 
if a file is already fully (de)compressed then eu-elfcompress can stop
early without needing to do any unnecessary updates.

Cheers,

Mark

Re: [PATCH] elfcompress: Don't rewrite file if no section data needs to be updated.

[Mark Wielaard]

On Mon, Jul 23, 2018 at 06:36:37PM +0200, Igor Gnatenko wrote:
> Acked-by: Igor Gnatenko <ignatenkobrain@fedoraproject.org>

Thanks. Pushed to master.

Re: [PATCH] elfcompress: Don't rewrite file if no section data needs to be updated.

[Mark Wielaard]

On Tue, Jul 24, 2018 at 09:57:28AM +0200, Mark Wielaard wrote:
> On Mon, Jul 23, 2018 at 06:36:37PM +0200, Igor Gnatenko wrote:
> > Acked-by: Igor Gnatenko <ignatenkobrain@fedoraproject.org>
> 
> Thanks. Pushed to master.

BTW. For those at home just reading the mailinglist and wondering why
they only see one part of the converstation. The elfutils-devel
mailinglist is open for all, but does reject HTML emails. But since I
was on CC I still got those, replying text-only.