Re: [patch/v2] simplify debugging in-container tests (and regular tests)

[Carlos O'Donell <carlos at redhat dot com>]

On 10/21/19 3:23 PM, DJ Delorie wrote:
> 
>> * Use a boolean WAIT_FOR_DEBUGGER supporting 1 or 0 value (true and false respectively).
> 
> Done.  Note: the test is still run as --direct if WAIT_FOR_DEBUGGER=0
> but only because the code is simpler that way ;-)

I'm OK with that. Thanks for spell that out.

>> * Have the child write a *.x file for gdb to just run. Simplifies things.
> 
> Done!
> 
>> Why is this volatile?
> 
> Comment added.
> 
>> Florian already mentioned a small usleep here. Just kinder on the host
>> system.
> 
> Added.  Note, however, that the test now almost always breaks deep
> inside usleep() instead of right in the support code.

That's OK, you break and continue anyway. We'll see what people say :-)

> 
> From 859b271b6f95762c25f018a7eac43180f7546ce9 Mon Sep 17 00:00:00 2001
> From: DJ Delorie <dj@redhat.com>
> Date: Wed, 2 Oct 2019 14:46:46 -0400
> Subject: Add wait-for-debugger test harness hooks
> 
> If WAIT_FOR_DEBUGGER is set to a non-zero value in the environment,
> any test that runs will print some useful gdb information and wait
> for gdb to attach to it and clear the "wait_for_debugger" variable.

This version looks good to me!

Please update:
https://sourceware.org/glibc/wiki/Testing/Builds

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> diff --git a/support/support_test_main.c b/support/support_test_main.c
> index 05ad92e688..0be1bd3d78 100644
> --- a/support/support_test_main.c
> +++ b/support/support_test_main.c
> @@ -19,6 +19,7 @@
>  #include <support/test-driver.h>
>  #include <support/check.h>
>  #include <support/temp_file-internal.h>
> +#include <support/support.h>

OK.

>  
>  #include <assert.h>
>  #include <errno.h>
> @@ -36,6 +37,8 @@
>  #include <time.h>
>  #include <unistd.h>
>  
> +#include <xstdio.h>
> +

OK.

>  static const struct option default_options[] =
>  {
>    TEST_DEFAULT_OPTIONS
> @@ -176,10 +179,55 @@ signal_handler (int sig)
>    exit (1);
>  }
>  
> +/* This must be volatile as it will be modified by the debugger.  */
> +static volatile int wait_for_debugger = 0;
> +

OK.

>  /* Run test_function or test_function_argv.  */
>  static int
>  run_test_function (int argc, char **argv, const struct test_config *config)
>  {
> +  const char *wfd = getenv("WAIT_FOR_DEBUGGER");
> +  if (wfd != NULL)
> +    wait_for_debugger = atoi (wfd);
> +  if (wait_for_debugger)
> +    {
> +      pid_t mypid;
> +      FILE *gdb_script;
> +      char *gdb_script_name;
> +      int inside_container = 0;
> +
> +      mypid = getpid();
> +      if (mypid < 3)
> +	{
> +	  const char *outside_pid = getenv("PID_OUTSIDE_CONTAINER");
> +	  if (outside_pid)
> +	    {
> +	      mypid = atoi (outside_pid);
> +	      inside_container = 1;
> +	    }
> +	}
> +
> +      gdb_script_name = (char *) xmalloc (strlen (argv[0]) + strlen (".gdb") + 1);
> +      sprintf (gdb_script_name, "%s.gdb", argv[0]);
> +      gdb_script = xfopen (gdb_script_name, "w");
> +
> +      fprintf (stderr, "Waiting for debugger, test process is pid %d\n", mypid);
> +      fprintf (stderr, "gdb -x %s\n", gdb_script_name);
> +      if (inside_container)
> +	fprintf (gdb_script, "set sysroot %s/testroot.root\n", support_objdir_root);
> +      fprintf (gdb_script, "file\n");
> +      fprintf (gdb_script, "file %s\n", argv[0]);
> +      fprintf (gdb_script, "symbol-file %s\n", argv[0]);
> +      fprintf (gdb_script, "exec-file %s\n", argv[0]);
> +      fprintf (gdb_script, "attach %ld\n", (long int) mypid);
> +      fprintf (gdb_script, "set wait_for_debugger = 0\n");
> +      fclose (gdb_script);

OK. Perfect!

> +    }
> +
> +  /* Wait for the debugger to set wait_for_debugger to zero.  */
> +  while (wait_for_debugger)
> +    usleep (1000);

OK.

> +
>    if (config->test_function != NULL)
>      return config->test_function ();
>    else if (config->test_function_argv != NULL)
> @@ -229,6 +277,11 @@ support_test_main (int argc, char **argv, const struct test_config *config)
>    unsigned int timeoutfactor = 1;
>    pid_t termpid;
>  
> +  /* If we're debugging the test, we need to disable timeouts and use
> +     the initial pid (esp if we're running inside a container).  */
> +  if (getenv("WAIT_FOR_DEBUGGER") != NULL)
> +    direct = 1;

OK.

> +
>    if (!config->no_mallopt)
>      {
>        /* Make uses of freed and uninitialized memory known.  Do not
> diff --git a/support/test-container.c b/support/test-container.c
> index 9c42d3ae2f..5d08979df3 100644
> --- a/support/test-container.c
> +++ b/support/test-container.c
> @@ -676,6 +676,9 @@ main (int argc, char **argv)
>    char *so_base;
>    int do_postclean = 0;
>  
> +  int pipes[2];
> +  char pid_buf[20];
> +
>    uid_t original_uid;
>    gid_t original_gid;
>    /* If set, the test runs as root instead of the user running the testsuite.  */
> @@ -999,6 +1002,11 @@ main (int argc, char **argv)
>    if (chdir (new_cwd_path) < 0)
>      FAIL_EXIT1 ("Can't cd to new %s - ", new_cwd_path);
>  
> +  /* This is to pass the "outside" PID to the child, which will be PID
> +     1.  */
> +  if (pipe2 (pipes, O_CLOEXEC) < 0)
> +    FAIL_EXIT1 ("Can't create pid pipe");
> +
>    /* To complete the containerization, we need to fork () at least
>       once.  We can't exec, nor can we somehow link the new child to
>       our parent.  So we run the child and propogate it's exit status
> @@ -1010,6 +1018,12 @@ main (int argc, char **argv)
>      {
>        /* Parent.  */
>        int status;
> +
> +      /* Send the child's "outside" pid to it.  */
> +      write (pipes[1], &child, sizeof(child));
> +      close (pipes[0]);
> +      close (pipes[1]);

OK.

> +
>        waitpid (child, &status, 0);
>  
>        if (WIFEXITED (status))
> @@ -1028,6 +1042,14 @@ main (int argc, char **argv)
>    /* The rest is the child process, which is now PID 1 and "in" the
>       new root.  */
>  
> +  /* Get our "outside" pid from our parent.  We use this to help with
> +     debugging from outside the container.  */
> +  read (pipes[0], &child, sizeof(child));
> +  close (pipes[0]);
> +  close (pipes[1]);
> +  sprintf (pid_buf, "%lu", (long unsigned)child);
> +  setenv ("PID_OUTSIDE_CONTAINER", pid_buf, 0);

OK.

> +
>    maybe_xmkdir ("/tmp", 0755);
>  
>    /* Now that we're pid 1 (effectively "root") we can mount /proc  */
> 


-- 
Cheers,
Carlos.