* [PATCH] Honour errors when processing more than one file @ 2019-01-01 0:00 Tom de Vries 2019-01-01 0:00 ` Jakub Jelinek 0 siblings, 1 reply; 4+ messages in thread From: Tom de Vries @ 2019-01-01 0:00 UTC (permalink / raw) To: dwz, jakub Hi, 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. OK for trunk? Thanks, - Tom Honour errors 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. --- dwz.c | 2 ++ testsuite/dwz.tests/two-files-too-many-dies.sh | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/dwz.c b/dwz.c index d348418..0c53c4b 100644 --- a/dwz.c +++ b/dwz.c @@ -11940,6 +11940,8 @@ main (int argc, char *argv[]) } else if (resa[i - optind].res == 0) successcount++; + if (thisret == 1) + return 1; if (hardlink && resa[i - optind].res >= 0 && resa[i - optind].nlink > 1) 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..7214ff6 --- /dev/null +++ b/testsuite/dwz.tests/two-files-too-many-dies.sh @@ -0,0 +1,19 @@ +#!/bin/sh + +set -e + +cp ../hello 1 +cp ../hello 2 + +if dwz -L0 1 2 2>/dev/null; then + exit 1 +fi + +cmp 1 ../hello +cmp 2 ../hello + +ls=$(ls) +ls=$(echo $ls) +[ "$ls" = "1 2" ] + +rm -f 1 2 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Honour errors when processing more than one file 2019-01-01 0:00 [PATCH] Honour errors when processing more than one file Tom de Vries @ 2019-01-01 0:00 ` Jakub Jelinek 2019-01-01 0:00 ` Tom de Vries 0 siblings, 1 reply; 4+ messages in thread From: Jakub Jelinek @ 2019-01-01 0:00 UTC (permalink / raw) To: Tom de Vries; +Cc: dwz 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. 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., 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, while if we already are in lowmem mode from the previous file, we count it. Jakub ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Honour errors when processing more than one file 2019-01-01 0:00 ` Jakub Jelinek @ 2019-01-01 0:00 ` Tom de Vries 2019-01-01 0:00 ` Tom de Vries 0 siblings, 1 reply; 4+ messages in thread From: Tom de Vries @ 2019-01-01 0:00 UTC (permalink / raw) To: Jakub Jelinek; +Cc: dwz [-- 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] Honour errors when processing more than one file 2019-01-01 0:00 ` Tom de Vries @ 2019-01-01 0:00 ` Tom de Vries 0 siblings, 0 replies; 4+ messages in thread From: Tom de Vries @ 2019-01-01 0:00 UTC (permalink / raw) To: Jakub Jelinek; +Cc: dwz [-- Attachment #1: Type: text/plain, Size: 3198 bytes --] On 05-03-19 21:32, Tom de Vries wrote: > 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? Committed as attached. Thanks, - Tom [-- Attachment #2: 0001-Honour-errors-when-processing-more-than-one-file.patch --] [-- Type: text/x-patch, Size: 2025 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-05-10 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/pr24171.sh: Update expected return status. * testsuite/dwz.tests/two-files-too-many-dies.sh: Same. --- dwz.c | 2 ++ testsuite/dwz.tests/pr24171.sh | 2 +- testsuite/dwz.tests/two-files-too-many-dies.sh | 6 +----- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/dwz.c b/dwz.c index 51300d3..6f34a0c 100644 --- a/dwz.c +++ b/dwz.c @@ -12291,6 +12291,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/testsuite/dwz.tests/pr24171.sh b/testsuite/dwz.tests/pr24171.sh index 0705489..31b2439 100644 --- a/testsuite/dwz.tests/pr24171.sh +++ b/testsuite/dwz.tests/pr24171.sh @@ -10,6 +10,6 @@ if ! grep -q "DW_AT_stmt_list not DW_FORM_sec_offset or DW_FORM_data4" dwz.err; exit 1 fi -[ $status -eq 0 ] +[ $status -eq 1 ] rm -f 1 2 dwz.err diff --git a/testsuite/dwz.tests/two-files-too-many-dies.sh b/testsuite/dwz.tests/two-files-too-many-dies.sh index fb7d496..13950ba 100644 --- a/testsuite/dwz.tests/two-files-too-many-dies.sh +++ b/testsuite/dwz.tests/two-files-too-many-dies.sh @@ -9,11 +9,7 @@ if ! grep -q "Too many DIEs, not optimizing" dwz.err; then exit 1 fi -if [ $status -eq 0 ]; then - echo "PR24301 workaround used" > dwz.info -else - [ $status -eq 1 ] -fi +[ $status -eq 1 ] cmp 1 $execs/hello cmp 2 $execs/hello ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-05-10 7:35 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-01-01 0:00 [PATCH] Honour errors when processing more than one file Tom de Vries 2019-01-01 0:00 ` Jakub Jelinek 2019-01-01 0:00 ` Tom de Vries 2019-01-01 0:00 ` Tom de Vries
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).