Re: [PATCH] Honour errors when processing more than one file

[Tom de Vries <tdevries@suse.de>]

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

On 04-03-19 18:09, Jakub Jelinek wrote:
> On Mon, Mar 04, 2019 at 05:49:26PM +0100, Tom de Vries wrote:
>> 2019-03-04  Tom de Vries  <tdevries@suse.de>
>>
>> 	PR dwz/24301
>> 	* dwz.c (main): Handle dwz returning 1 if processing more than one file.
>> 	* testsuite/dwz.tests/two-files-too-many-dies.sh: New test.
> 
> That looks wrong to me, that means once an error is reported for any single
> one, the rest will not be processed.  That was not the intent and is
> undesirable for the distro compression, we want to compress as much as
> possible.
> 

I see, understood. Updated the test-case accordingly.

> Guess that
>           else if (resa[i - optind].res == 0)
>             successcount++;
> should be
>           if (resa[i - optind].res == 0)
>             successcount++;
> 	  else
> 	    ret = 1;
> or something similar, I must say I don't really remember the difference
> between res->res value and return value from dwz etc.,

The return value of the dwz function indicates whether a problem
occurred during dwz execution.

The res->res value, as far as I can infer from the source, is related to
file status, and can be (as added in the patch for PR24275):
...
  /* -2: Already processed under different name.
     -1: Ignore.
      0: Processed, changed.
      1: Processed, unchanged.
...
and unchanged can be either due to an error, or due to not being able to
compress.

[ The PR24275 is about that fact that dwz never in fact returns with
res->res set to 1. ]

> but it seems weird
> that if if some file isn't processed in normal mode and when switched into lowmem
> mode succeeds, we don't count it as successcount,

The successcount variable tracks the amount of unique files (that is, in
hardlink mode we count two hardlinks to the same file as one) that
succeeded in regular mode, which, if >=2 enables multifile mode.

AFAIU, the fact that low-mem files are not optimized in multifile mode
is listed explicitly in the file header documentation:
...
   If some executable or shared library has too large debug information

   (number of DIEs in .debug_info section) that there would be

   risk of too high memory consumption, that file isn't multifile

   optimized, instead it is processed by dwz () in a low-memory mode

   with low_mem flag set.  This can decrease memory consumption to

   half in some very large cases.  */
...

So, I'd say we want to keep the else in:
...
           else if (resa[i - optind].res == 0)
...

> while if we already are in
> lowmem mode from the previous file, we count it.

The lowmem mode is a per-file mode. If a previous file was processed in
low-mem mode, we still try the current file again in regular mode,
before possibly switching back to low-mem mode.

---

Also, we cannot use and else here:
...
 	  else
 	    ret = 1;
...
because that also triggers for res->res == -2 (which would make
hardlink.sh fail). So I use:
...
         else if (thisret == 1)
...
instead.

OK like this?

Thanks,
- Tom

[-- Attachment #2: 0001-Honour-errors-when-processing-more-than-one-file.patch --]
[-- Type: text/x-patch, Size: 2436 bytes --]

Honour errors when processing more than one file

When using dwz -L0 on a hello world a.out, it fails:
...
$ dwz -L0 a.out
$ echo $?
1
...

But when we do the same for a.out and a copy b.out, it passes:
...
$ cp a.out b.out
$ dwz -L0 a.out b.out
$ echo $?
0
...

Fix this by honouring dwz return codes when processing more than one file.

2019-03-04  Tom de Vries  <tdevries@suse.de>

	PR dwz/24301
	* dwz.c (main): Handle dwz returning 1 if processing more than one file.
	* testsuite/dwz.tests/two-files-too-many-dies.sh: New test.

---
 Makefile                                       |  5 ++++-
 dwz.c                                          |  2 ++
 hello.c                                        | 14 ++++++++++++++
 testsuite/dwz.tests/two-files-too-many-dies.sh | 22 ++++++++++++++++++++++
 4 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index b318fd7..4b37732 100644
--- a/Makefile
+++ b/Makefile
@@ -17,11 +17,14 @@ clean:
 
 PWD:=$(shell pwd -P)
 
-TEST_EXECS = hello
+TEST_EXECS = hello hello2
 
 hello:
 	$(CC) hello.c -o $@ -g
 
+hello2:
+	$(CC) hello.c -o $@ -DWITH_STRUCT -g
+
 check: dwz $(TEST_EXECS)
 	mkdir -p testsuite-bin
 	cd testsuite-bin; ln -sf $(PWD)/dwz .
diff --git a/dwz.c b/dwz.c
index d348418..e82d6a7 100644
--- a/dwz.c
+++ b/dwz.c
@@ -11940,6 +11940,8 @@ main (int argc, char *argv[])
 	    }
 	  else if (resa[i - optind].res == 0)
 	    successcount++;
+	  else if (thisret == 1)
+	    ret = 1;
 	  if (hardlink
 	      && resa[i - optind].res >= 0
 	      && resa[i - optind].nlink > 1)
diff --git a/hello.c b/hello.c
index 82d070e..ee65515 100644
--- a/hello.c
+++ b/hello.c
@@ -1,8 +1,22 @@
 #include <stdio.h>
 
+#ifdef WITH_STRUCT
+struct a {
+  int a;
+  char *p;
+};
+
+struct a a;
+#endif
+
 int
 main (void)
 {
+#ifdef WITH_STRUCT
+  a.p = "hello";
+  printf ("%s\n", a.p);
+#else
   printf ("hello\n");
+#endif
   return 0;
 }
diff --git a/testsuite/dwz.tests/two-files-too-many-dies.sh b/testsuite/dwz.tests/two-files-too-many-dies.sh
new file mode 100755
index 0000000..da86d23
--- /dev/null
+++ b/testsuite/dwz.tests/two-files-too-many-dies.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+set -e
+
+cp ../hello 1
+cp ../hello2 2
+
+limit=$(readelf -w 2 | grep '(DW_TAG' | wc -l)
+limit=$((limit - 1))
+
+if dwz -L$limit 2 1 2>/dev/null; then
+    exit 1
+fi
+
+smaller-than.sh 1 ../hello
+cmp 2 ../hello2
+
+ls=$(ls)
+ls=$(echo $ls)
+[ "$ls" = "1 2" ]
+
+rm -f 1 2