* [PATCH] selftest: invoke "diff" when ASSERT_STREQ fails @ 2024-05-17 19:51 David Malcolm 2024-05-28 17:20 ` PING: " David Malcolm 0 siblings, 1 reply; 5+ messages in thread From: David Malcolm @ 2024-05-17 19:51 UTC (permalink / raw) To: gcc-patches; +Cc: David Malcolm Currently when ASSERT_STREQ or ASSERT_STREQ_AT fail we print both strings to stderr. However it can be hard to figure out the problem (e.g. for 1-character differences in long strings). Extend the output by writing out the strings to tempfiles and invoking "diff -up" on them when we have such a selftest failure, to (I hope) simplify debugging. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. OK for trunk? gcc/ChangeLog: * selftest.cc (selftest::print_diff): New function. (selftest::assert_streq): Call it when we have non-equal non-null strings. Signed-off-by: David Malcolm <dmalcolm@redhat.com> --- gcc/selftest.cc | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/gcc/selftest.cc b/gcc/selftest.cc index 6438d86a6aa0..f58c0631908e 100644 --- a/gcc/selftest.cc +++ b/gcc/selftest.cc @@ -63,6 +63,26 @@ fail_formatted (const location &loc, const char *fmt, ...) abort (); } +/* Invoke "diff" to print the difference between VAL1 and VAL2 + on stdout. */ + +static void +print_diff (const location &loc, const char *val1, const char *val2) +{ + temp_source_file tmpfile1 (loc, ".txt", val1); + temp_source_file tmpfile2 (loc, ".txt", val2); + const char *args[] = {"diff", + "-up", + tmpfile1.get_filename (), + tmpfile2.get_filename (), + NULL}; + int exit_status = 0; + int err = 0; + pex_one (PEX_SEARCH | PEX_LAST, + args[0], CONST_CAST (char **, args), + NULL, NULL, NULL, &exit_status, &err); +} + /* Implementation detail of ASSERT_STREQ. Compare val1 and val2 with strcmp. They ought to be non-NULL; fail gracefully if either or both are NULL. */ @@ -89,8 +109,12 @@ assert_streq (const location &loc, if (strcmp (val1, val2) == 0) pass (loc, "ASSERT_STREQ"); else - fail_formatted (loc, "ASSERT_STREQ (%s, %s)\n val1=\"%s\"\n val2=\"%s\"\n", - desc_val1, desc_val2, val1, val2); + { + print_diff (loc, val1, val2); + fail_formatted + (loc, "ASSERT_STREQ (%s, %s)\n val1=\"%s\"\n val2=\"%s\"\n", + desc_val1, desc_val2, val1, val2); + } } } -- 2.26.3 ^ permalink raw reply [flat|nested] 5+ messages in thread
* PING: Re: [PATCH] selftest: invoke "diff" when ASSERT_STREQ fails 2024-05-17 19:51 [PATCH] selftest: invoke "diff" when ASSERT_STREQ fails David Malcolm @ 2024-05-28 17:20 ` David Malcolm 2024-05-29 20:35 ` Eric Gallager 0 siblings, 1 reply; 5+ messages in thread From: David Malcolm @ 2024-05-28 17:20 UTC (permalink / raw) To: gcc-patches Ping. This patch has actually been *very* helpful to me when debugging selftest failures involving ASSERT_STREQ. Thanks Dave On Fri, 2024-05-17 at 15:51 -0400, David Malcolm wrote: > Currently when ASSERT_STREQ or ASSERT_STREQ_AT fail we print > both strings to stderr. However it can be hard to figure out > the problem (e.g. for 1-character differences in long strings). > > Extend the output by writing out the strings to tempfiles and > invoking "diff -up" on them when we have such a selftest failure, > to (I hope) simplify debugging. > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. > > OK for trunk? > > gcc/ChangeLog: > * selftest.cc (selftest::print_diff): New function. > (selftest::assert_streq): Call it when we have non-equal > non-null strings. > > Signed-off-by: David Malcolm <dmalcolm@redhat.com> > --- > gcc/selftest.cc | 28 ++++++++++++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/gcc/selftest.cc b/gcc/selftest.cc > index 6438d86a6aa0..f58c0631908e 100644 > --- a/gcc/selftest.cc > +++ b/gcc/selftest.cc > @@ -63,6 +63,26 @@ fail_formatted (const location &loc, const char > *fmt, ...) > abort (); > } > > +/* Invoke "diff" to print the difference between VAL1 and VAL2 > + on stdout. */ > + > +static void > +print_diff (const location &loc, const char *val1, const char *val2) > +{ > + temp_source_file tmpfile1 (loc, ".txt", val1); > + temp_source_file tmpfile2 (loc, ".txt", val2); > + const char *args[] = {"diff", > + "-up", > + tmpfile1.get_filename (), > + tmpfile2.get_filename (), > + NULL}; > + int exit_status = 0; > + int err = 0; > + pex_one (PEX_SEARCH | PEX_LAST, > + args[0], CONST_CAST (char **, args), > + NULL, NULL, NULL, &exit_status, &err); > +} > + > /* Implementation detail of ASSERT_STREQ. > Compare val1 and val2 with strcmp. They ought > to be non-NULL; fail gracefully if either or both are NULL. */ > @@ -89,8 +109,12 @@ assert_streq (const location &loc, > if (strcmp (val1, val2) == 0) > pass (loc, "ASSERT_STREQ"); > else > - fail_formatted (loc, "ASSERT_STREQ (%s, %s)\n val1=\"%s\"\n > val2=\"%s\"\n", > - desc_val1, desc_val2, val1, val2); > + { > + print_diff (loc, val1, val2); > + fail_formatted > + (loc, "ASSERT_STREQ (%s, %s)\n val1=\"%s\"\n > val2=\"%s\"\n", > + desc_val1, desc_val2, val1, val2); > + } > } > } > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: PING: Re: [PATCH] selftest: invoke "diff" when ASSERT_STREQ fails 2024-05-28 17:20 ` PING: " David Malcolm @ 2024-05-29 20:35 ` Eric Gallager 2024-05-29 21:06 ` David Malcolm 0 siblings, 1 reply; 5+ messages in thread From: Eric Gallager @ 2024-05-29 20:35 UTC (permalink / raw) To: David Malcolm; +Cc: gcc-patches On Tue, May 28, 2024 at 1:21 PM David Malcolm <dmalcolm@redhat.com> wrote: > > Ping. > > This patch has actually been *very* helpful to me when debugging > selftest failures involving ASSERT_STREQ. > > Thanks > Dave > Currently `diff` is only listed under the "Tools/packages necessary for modifying GCC" section of install/prerequisites.html: https://gcc.gnu.org/install/prerequisites.html If it's going to become a dependency for actually running GCC, too, it should get moved to be documented elsewhere, IMO. > On Fri, 2024-05-17 at 15:51 -0400, David Malcolm wrote: > > Currently when ASSERT_STREQ or ASSERT_STREQ_AT fail we print > > both strings to stderr. However it can be hard to figure out > > the problem (e.g. for 1-character differences in long strings). > > > > Extend the output by writing out the strings to tempfiles and > > invoking "diff -up" on them when we have such a selftest failure, > > to (I hope) simplify debugging. > > > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. > > > > OK for trunk? > > > > gcc/ChangeLog: > > * selftest.cc (selftest::print_diff): New function. > > (selftest::assert_streq): Call it when we have non-equal > > non-null strings. > > > > Signed-off-by: David Malcolm <dmalcolm@redhat.com> > > --- > > gcc/selftest.cc | 28 ++++++++++++++++++++++++++-- > > 1 file changed, 26 insertions(+), 2 deletions(-) > > > > diff --git a/gcc/selftest.cc b/gcc/selftest.cc > > index 6438d86a6aa0..f58c0631908e 100644 > > --- a/gcc/selftest.cc > > +++ b/gcc/selftest.cc > > @@ -63,6 +63,26 @@ fail_formatted (const location &loc, const char > > *fmt, ...) > > abort (); > > } > > > > +/* Invoke "diff" to print the difference between VAL1 and VAL2 > > + on stdout. */ > > + > > +static void > > +print_diff (const location &loc, const char *val1, const char *val2) > > +{ > > + temp_source_file tmpfile1 (loc, ".txt", val1); > > + temp_source_file tmpfile2 (loc, ".txt", val2); > > + const char *args[] = {"diff", > > + "-up", > > + tmpfile1.get_filename (), > > + tmpfile2.get_filename (), > > + NULL}; > > + int exit_status = 0; > > + int err = 0; > > + pex_one (PEX_SEARCH | PEX_LAST, > > + args[0], CONST_CAST (char **, args), > > + NULL, NULL, NULL, &exit_status, &err); > > +} > > + > > /* Implementation detail of ASSERT_STREQ. > > Compare val1 and val2 with strcmp. They ought > > to be non-NULL; fail gracefully if either or both are NULL. */ > > @@ -89,8 +109,12 @@ assert_streq (const location &loc, > > if (strcmp (val1, val2) == 0) > > pass (loc, "ASSERT_STREQ"); > > else > > - fail_formatted (loc, "ASSERT_STREQ (%s, %s)\n val1=\"%s\"\n > > val2=\"%s\"\n", > > - desc_val1, desc_val2, val1, val2); > > + { > > + print_diff (loc, val1, val2); > > + fail_formatted > > + (loc, "ASSERT_STREQ (%s, %s)\n val1=\"%s\"\n > > val2=\"%s\"\n", > > + desc_val1, desc_val2, val1, val2); > > + } > > } > > } > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: PING: Re: [PATCH] selftest: invoke "diff" when ASSERT_STREQ fails 2024-05-29 20:35 ` Eric Gallager @ 2024-05-29 21:06 ` David Malcolm 2024-06-26 9:10 ` Eric Gallager 0 siblings, 1 reply; 5+ messages in thread From: David Malcolm @ 2024-05-29 21:06 UTC (permalink / raw) To: Eric Gallager; +Cc: gcc-patches On Wed, 2024-05-29 at 16:35 -0400, Eric Gallager wrote: > On Tue, May 28, 2024 at 1:21 PM David Malcolm <dmalcolm@redhat.com> > wrote: > > > > Ping. > > > > This patch has actually been *very* helpful to me when debugging > > selftest failures involving ASSERT_STREQ. > > > > Thanks > > Dave > > > > Currently `diff` is only listed under the "Tools/packages necessary > for modifying GCC" section of install/prerequisites.html: > https://gcc.gnu.org/install/prerequisites.html > If it's going to become a dependency for actually running GCC, too, > it > should get moved to be documented elsewhere, IMO. All this is selftest code, and is turned off in a release configuration of GCC. The code path that invokes "diff" is when a selftest is failing, which is immediately before a hard failure of the *build* of GCC. So arguably this is just a build-time thing for people packaging/hacking on GCC, and thus not a new dependency for end-usage. BTW I'm a bit hazy on the details of how "pex" is meant to work, so hopefully someone more knowledgable than me can comment on that aspect of the patch. It seems to work though. Dave > > > On Fri, 2024-05-17 at 15:51 -0400, David Malcolm wrote: > > > Currently when ASSERT_STREQ or ASSERT_STREQ_AT fail we print > > > both strings to stderr. However it can be hard to figure out > > > the problem (e.g. for 1-character differences in long strings). > > > > > > Extend the output by writing out the strings to tempfiles and > > > invoking "diff -up" on them when we have such a selftest failure, > > > to (I hope) simplify debugging. > > > > > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. > > > > > > OK for trunk? > > > > > > gcc/ChangeLog: > > > * selftest.cc (selftest::print_diff): New function. > > > (selftest::assert_streq): Call it when we have non-equal > > > non-null strings. > > > > > > Signed-off-by: David Malcolm <dmalcolm@redhat.com> > > > --- > > > gcc/selftest.cc | 28 ++++++++++++++++++++++++++-- > > > 1 file changed, 26 insertions(+), 2 deletions(-) > > > > > > diff --git a/gcc/selftest.cc b/gcc/selftest.cc > > > index 6438d86a6aa0..f58c0631908e 100644 > > > --- a/gcc/selftest.cc > > > +++ b/gcc/selftest.cc > > > @@ -63,6 +63,26 @@ fail_formatted (const location &loc, const > > > char > > > *fmt, ...) > > > abort (); > > > } > > > > > > +/* Invoke "diff" to print the difference between VAL1 and VAL2 > > > + on stdout. */ > > > + > > > +static void > > > +print_diff (const location &loc, const char *val1, const char > > > *val2) > > > +{ > > > + temp_source_file tmpfile1 (loc, ".txt", val1); > > > + temp_source_file tmpfile2 (loc, ".txt", val2); > > > + const char *args[] = {"diff", > > > + "-up", > > > + tmpfile1.get_filename (), > > > + tmpfile2.get_filename (), > > > + NULL}; > > > + int exit_status = 0; > > > + int err = 0; > > > + pex_one (PEX_SEARCH | PEX_LAST, > > > + args[0], CONST_CAST (char **, args), > > > + NULL, NULL, NULL, &exit_status, &err); > > > +} > > > + > > > /* Implementation detail of ASSERT_STREQ. > > > Compare val1 and val2 with strcmp. They ought > > > to be non-NULL; fail gracefully if either or both are NULL. > > > */ > > > @@ -89,8 +109,12 @@ assert_streq (const location &loc, > > > if (strcmp (val1, val2) == 0) > > > pass (loc, "ASSERT_STREQ"); > > > else > > > - fail_formatted (loc, "ASSERT_STREQ (%s, %s)\n > > > val1=\"%s\"\n > > > val2=\"%s\"\n", > > > - desc_val1, desc_val2, val1, val2); > > > + { > > > + print_diff (loc, val1, val2); > > > + fail_formatted > > > + (loc, "ASSERT_STREQ (%s, %s)\n val1=\"%s\"\n > > > val2=\"%s\"\n", > > > + desc_val1, desc_val2, val1, val2); > > > + } > > > } > > > } > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: PING: Re: [PATCH] selftest: invoke "diff" when ASSERT_STREQ fails 2024-05-29 21:06 ` David Malcolm @ 2024-06-26 9:10 ` Eric Gallager 0 siblings, 0 replies; 5+ messages in thread From: Eric Gallager @ 2024-06-26 9:10 UTC (permalink / raw) To: David Malcolm; +Cc: gcc-patches, Ian Lance Taylor On Wed, May 29, 2024 at 5:06 PM David Malcolm <dmalcolm@redhat.com> wrote: > > On Wed, 2024-05-29 at 16:35 -0400, Eric Gallager wrote: > > On Tue, May 28, 2024 at 1:21 PM David Malcolm <dmalcolm@redhat.com> > > wrote: > > > > > > Ping. > > > > > > This patch has actually been *very* helpful to me when debugging > > > selftest failures involving ASSERT_STREQ. > > > > > > Thanks > > > Dave > > > > > > > Currently `diff` is only listed under the "Tools/packages necessary > > for modifying GCC" section of install/prerequisites.html: > > https://gcc.gnu.org/install/prerequisites.html > > If it's going to become a dependency for actually running GCC, too, > > it > > should get moved to be documented elsewhere, IMO. > > All this is selftest code, and is turned off in a release configuration > of GCC. The code path that invokes "diff" is when a selftest is > failing, which is immediately before a hard failure of the *build* of > GCC. So arguably this is just a build-time thing for people > packaging/hacking on GCC, and thus not a new dependency for end-usage. > Well to be clear, I'm just asking for a documentation update here, so if you want to use wording that reflects all of that, I think that'd be fine. It seems like a useful idea overall, so don't let my documentation request hold you up from proceeding with it. > BTW I'm a bit hazy on the details of how "pex" is meant to work, so > hopefully someone more knowledgable than me can comment on that aspect > of the patch. It seems to work though. > I'm not too clear on it either; maybe one of the libiberty maintainers can chime in? Or wait, looks like there's just the one currently (Ian); cc-ing... > Dave > > > > > > On Fri, 2024-05-17 at 15:51 -0400, David Malcolm wrote: > > > > Currently when ASSERT_STREQ or ASSERT_STREQ_AT fail we print > > > > both strings to stderr. However it can be hard to figure out > > > > the problem (e.g. for 1-character differences in long strings). > > > > > > > > Extend the output by writing out the strings to tempfiles and > > > > invoking "diff -up" on them when we have such a selftest failure, > > > > to (I hope) simplify debugging. > > > > > > > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. > > > > > > > > OK for trunk? > > > > > > > > gcc/ChangeLog: > > > > * selftest.cc (selftest::print_diff): New function. > > > > (selftest::assert_streq): Call it when we have non-equal > > > > non-null strings. > > > > > > > > Signed-off-by: David Malcolm <dmalcolm@redhat.com> > > > > --- > > > > gcc/selftest.cc | 28 ++++++++++++++++++++++++++-- > > > > 1 file changed, 26 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/gcc/selftest.cc b/gcc/selftest.cc > > > > index 6438d86a6aa0..f58c0631908e 100644 > > > > --- a/gcc/selftest.cc > > > > +++ b/gcc/selftest.cc > > > > @@ -63,6 +63,26 @@ fail_formatted (const location &loc, const > > > > char > > > > *fmt, ...) > > > > abort (); > > > > } > > > > > > > > +/* Invoke "diff" to print the difference between VAL1 and VAL2 > > > > + on stdout. */ > > > > + > > > > +static void > > > > +print_diff (const location &loc, const char *val1, const char > > > > *val2) > > > > +{ > > > > + temp_source_file tmpfile1 (loc, ".txt", val1); > > > > + temp_source_file tmpfile2 (loc, ".txt", val2); > > > > + const char *args[] = {"diff", > > > > + "-up", > > > > + tmpfile1.get_filename (), > > > > + tmpfile2.get_filename (), > > > > + NULL}; > > > > + int exit_status = 0; > > > > + int err = 0; > > > > + pex_one (PEX_SEARCH | PEX_LAST, > > > > + args[0], CONST_CAST (char **, args), > > > > + NULL, NULL, NULL, &exit_status, &err); > > > > +} > > > > + > > > > /* Implementation detail of ASSERT_STREQ. > > > > Compare val1 and val2 with strcmp. They ought > > > > to be non-NULL; fail gracefully if either or both are NULL. > > > > */ > > > > @@ -89,8 +109,12 @@ assert_streq (const location &loc, > > > > if (strcmp (val1, val2) == 0) > > > > pass (loc, "ASSERT_STREQ"); > > > > else > > > > - fail_formatted (loc, "ASSERT_STREQ (%s, %s)\n > > > > val1=\"%s\"\n > > > > val2=\"%s\"\n", > > > > - desc_val1, desc_val2, val1, val2); > > > > + { > > > > + print_diff (loc, val1, val2); > > > > + fail_formatted > > > > + (loc, "ASSERT_STREQ (%s, %s)\n val1=\"%s\"\n > > > > val2=\"%s\"\n", > > > > + desc_val1, desc_val2, val1, val2); > > > > + } > > > > } > > > > } > > > > > > > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-06-26 9:10 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-05-17 19:51 [PATCH] selftest: invoke "diff" when ASSERT_STREQ fails David Malcolm 2024-05-28 17:20 ` PING: " David Malcolm 2024-05-29 20:35 ` Eric Gallager 2024-05-29 21:06 ` David Malcolm 2024-06-26 9:10 ` Eric Gallager
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).