[PATCH] selftest: invoke "diff" when ASSERT_STREQ fails

[PATCH] selftest: invoke "diff" when ASSERT_STREQ fails

[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

PING: Re: [PATCH] selftest: invoke "diff" when ASSERT_STREQ fails

[David Malcolm]

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);
> +         }
>        }
>  }
>  

Re: PING: Re: [PATCH] selftest: invoke "diff" when ASSERT_STREQ fails

[Eric Gallager]

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);
> > +         }
> >        }
> >  }
> >
>

Re: PING: Re: [PATCH] selftest: invoke "diff" when ASSERT_STREQ fails

[David Malcolm]

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);
> > > +         }
> > >        }
> > >  }
> > > 
> > 
>