This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Fix BZ#20544 (assert function passed to atexit/on_exit/__cxa_atexit != NULL)



On 28/11/2018 17:07, Florian Weimer wrote:
> * Paul Pluzhnikov:
> 
>> On Mon, Nov 26, 2018 at 11:02 AM Adhemerval Zanella
>> <adhemerval.zanella@linaro.org> wrote:
>>
>> Thanks for the review.
>>
>>> #else
>>>   {
>>>     struct support_capture_subprocess result;
>>>     result = support_capture_subprocess (do_test_bz20544_atexit, NULL);
>>>     support_capture_subprocess_check (&result, "bz20544", 128 + SIGABRT,
>>>                                       sc_allow_stderr);
>>
>> This doesn't work: the actual exit code on my Linux/x86_64 system is 6, not 134.
>>
>> I notice that in libio/tst-vtables-common.c, Florian first initialized
>> expected SIGABRT termination_status in init_termination_status(), and
>> then used that in support_capture_subprocess_check() calls. Do I need
>> to do the same here?
> 
> You can try the patch posted below.  The initial API design was a bit
> lazy.
> 
>> A different way to ask: do different OSes encode WIFSIGNALED /
>> WIFEXITED differently?
> 
> Yes, but all glibc targets currently behave the same.  I don't expect
> this to change because the a lot of things are hard-coded (see the
> JRuby/JVM discussion on reporting signal termination).

Do you have a link for the discussion?

> 
> We should probably add macros to go into the other direction to
> <sys/wait.h>, given that the layout is unlikely to change, ever.

Is is something usually required and/or reimplemented elsewhere?

> 
> Thanks,
> Florian
> 
> -----
> support: Add signal support to support_capture_subprocess_check
> 
> Adjust libio/tst-vtables-common.c to use this new functionality,
> instead of determining the termination status for a signal indirectly.
> 
> 2018-11-28  Florian Weimer  <fweimer@redhat.com>
> 
> 	support: Add signal support to support_capture_subprocess_check.
> 	* support/capture_subprocess.h (support_capture_subprocess_check):
> 	Adjust comment and rename parameter.
> 	* support/support_capture_subprocess_check.c
> 	(print_actual_status): New function.
> 	(support_capture_subprocess_check): Support negative
> 	status_or_signal.  Call print_actual_status.
> 	* support/tst-support_capture_subprocess.c (do_test): Call
> 	support_capture_subprocess_check.
> 	* libio/tst-vtables-common.c (termination_status)
> 	(init_termination_status): Remove.
> 	(check_for_termination): Adjust support_capture_subprocess_check
> 	call.
> 	(do_test): Remove call to init_termination_status.

LGTM, thanks.

> 
> diff --git a/libio/tst-vtables-common.c b/libio/tst-vtables-common.c
> index 5e31012069..85e246cd11 100644
> --- a/libio/tst-vtables-common.c
> +++ b/libio/tst-vtables-common.c
> @@ -380,21 +380,6 @@ without_compatibility_fflush (void *closure)
>    _exit (1);
>  }
>  
> -/* Exit status after abnormal termination.  */
> -static int termination_status;
> -
> -static void
> -init_termination_status (void)
> -{
> -  pid_t pid = xfork ();
> -  if (pid == 0)
> -    abort ();
> -  xwaitpid (pid, &termination_status, 0);
> -
> -  TEST_VERIFY (WIFSIGNALED (termination_status));
> -  TEST_COMPARE (WTERMSIG (termination_status), SIGABRT);
> -}
> -
>  static void
>  check_for_termination (const char *name, void (*callback) (void *))
>  {
> @@ -404,7 +389,7 @@ check_for_termination (const char *name, void (*callback) (void *))
>    shared->calls = 0;
>    struct support_capture_subprocess proc
>      = support_capture_subprocess (callback, NULL);
> -  support_capture_subprocess_check (&proc, name, termination_status,
> +  support_capture_subprocess_check (&proc, name, -SIGABRT,
>                                      sc_allow_stderr);
>    const char *message
>      = "Fatal error: glibc detected an invalid stdio handle\n";
> @@ -491,7 +476,6 @@ run_tests (bool initially_disabled)
>  
>    shared = support_shared_allocate (sizeof (*shared));
>    shared->initially_disabled = initially_disabled;
> -  init_termination_status ();
>  
>    if (initially_disabled)
>      {

Ok.

> diff --git a/support/capture_subprocess.h b/support/capture_subprocess.h
> index b0886ba1d1..4734b877ad 100644
> --- a/support/capture_subprocess.h
> +++ b/support/capture_subprocess.h
> @@ -49,13 +49,16 @@ enum support_capture_allow
>    sc_allow_stderr = 0x04,
>  };
>  
> -/* Check that the subprocess exited with STATUS and that only the
> -   allowed outputs happened.  ALLOWED is a combination of
> -   support_capture_allow flags.  Report errors under the CONTEXT
> -   message.  */
> +/* Check that the subprocess exited and that only the allowed outputs
> +   happened.  If STATUS_OR_SIGNAL is nonnegative, it is the expected
> +   (decoded) exit status of the process, as returned by WEXITSTATUS.
> +   If STATUS_OR_SIGNAL is negative, -STATUS_OR_SIGNAL is the expected
> +   termination signal, as returned by WTERMSIG.  ALLOWED is a
> +   combination of support_capture_allow flags.  Report errors under
> +   the CONTEXT message.  */
>  void support_capture_subprocess_check (struct support_capture_subprocess *,
> -                                       const char *context, int status,
> -                                       int allowed)
> +                                       const char *context,
> +                                       int status_or_signal, int allowed)
>    __attribute__ ((nonnull (1, 2)));
>  
>  #endif /* SUPPORT_CAPTURE_SUBPROCESS_H */

Ok.

> diff --git a/support/support_capture_subprocess_check.c b/support/support_capture_subprocess_check.c
> index ff5ee89fb0..8b4c352c96 100644
> --- a/support/support_capture_subprocess_check.c
> +++ b/support/support_capture_subprocess_check.c
> @@ -20,6 +20,7 @@
>  #include <stdio.h>
>  #include <support/capture_subprocess.h>
>  #include <support/check.h>
> +#include <sys/wait.h>
>  
>  static void
>  print_context (const char *context, bool *failed)
> @@ -31,9 +32,22 @@ print_context (const char *context, bool *failed)
>    printf ("error: subprocess failed: %s\n", context);
>  }
>  
> +static void
> +print_actual_status (struct support_capture_subprocess *proc)
> +{
> +  if (WIFEXITED (proc->status))
> +    printf ("error:   actual exit status: %d [0x%x]\n",
> +            WEXITSTATUS (proc->status), proc->status);
> +  else if (WIFSIGNALED (proc->status))
> +    printf ("error:   actual termination signal: %d [0x%x]\n",
> +            WTERMSIG (proc->status), proc->status);
> +  else
> +    printf ("error:   actual undecoded exit status: [0x%x]\n", proc->status);
> +}
> +>  void
>  support_capture_subprocess_check (struct support_capture_subprocess *proc,
> -                                  const char *context, int status,
> +                                  const char *context, int status_or_signal,
>                                    int allowed)
>  {
>    TEST_VERIFY ((allowed & sc_allow_none)
> @@ -44,11 +58,28 @@ support_capture_subprocess_check (struct support_capture_subprocess *proc,
>                       || (allowed & sc_allow_stderr))));
>  
>    bool failed = false;
> -  if (proc->status != status)
> +  if (status_or_signal >= 0)
>      {
> -      print_context (context, &failed);
> -      printf ("error:   expected exit status: %d\n", status);
> -      printf ("error:   actual exit status:   %d\n", proc->status);
> +      /* Expect regular termination.  */
> +      if (!(WIFEXITED (proc->status)
> +            && WEXITSTATUS (proc->status) == status_or_signal))
> +        {
> +          print_context (context, &failed);
> +          printf ("error:   expected exit status: %d\n", status_or_signal);
> +          print_actual_status (proc);
> +        }
> +    }
> +  else
> +    {
> +      /* status_or_signal < 0.  Expect termination by signal.  */
> +      if (!(WIFSIGNALED (proc->status)
> +            && WTERMSIG (proc->status) == -status_or_signal))
> +        {
> +          print_context (context, &failed);
> +          printf ("error:   expected termination signal: %d\n",
> +                  -status_or_signal);
> +          print_actual_status (proc);
> +        }
>      }
>    if (!(allowed & sc_allow_stdout) && proc->out.length != 0)
>      {

Ok.

> diff --git a/support/tst-support_capture_subprocess.c b/support/tst-support_capture_subprocess.c
> index a685256091..5339e85b07 100644
> --- a/support/tst-support_capture_subprocess.c
> +++ b/support/tst-support_capture_subprocess.c
> @@ -168,15 +168,29 @@ do_test (void)
>                  = support_capture_subprocess (callback, &test);
>                check_stream ("stdout", &result.out, test.out);
>                check_stream ("stderr", &result.err, test.err);
> +
> +              /* Allowed output for support_capture_subprocess_check.  */
> +              int check_allow = 0;
> +              if (lengths[length_idx_stdout] > 0)
> +                check_allow |= sc_allow_stdout;
> +              if (lengths[length_idx_stderr] > 0)
> +                check_allow |= sc_allow_stderr;
> +              if (check_allow == 0)
> +                check_allow = sc_allow_none;
> +
>                if (test.signal != 0)
>                  {
>                    TEST_VERIFY (WIFSIGNALED (result.status));
>                    TEST_VERIFY (WTERMSIG (result.status) == test.signal);
> +                  support_capture_subprocess_check (&result, "signal",
> +                                                    -SIGTERM, check_allow);
>                  }
>                else
>                  {
>                    TEST_VERIFY (WIFEXITED (result.status));
>                    TEST_VERIFY (WEXITSTATUS (result.status) == test.status);
> +                  support_capture_subprocess_check (&result, "exit",
> +                                                    test.status, check_allow);
>                  }
>                support_capture_subprocess_free (&result);
>                free (test.out);
> 

Ok.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]