[PATCH v1] cpukit/libmisc/capture: fix capture bug

[Gedare Bloom]

On Mon, May 23, 2022 at 1:40 AM <tianye at sugon.com> wrote:
>
> From: Tian Ye <tianye at sugon.com>
>
What is the bug this fixes? Is there an open ticket for it?

> ---
>  cpukit/libmisc/capture/capture.c         | 30 ++++++++++++++++++++----
>  cpukit/libmisc/capture/capture_support.c |  8 +++++--
>  2 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/cpukit/libmisc/capture/capture.c b/cpukit/libmisc/capture/capture.c
> index 2228d135b8..886b4f62ed 100644
> --- a/cpukit/libmisc/capture/capture.c
> +++ b/cpukit/libmisc/capture/capture.c
> @@ -292,6 +292,23 @@ rtems_capture_find_control (rtems_name name, rtems_id id)
>    return control;
>  }
>
> +static bool
> +rtems_capture_uninitialize_control (rtems_tcb *tcb, void *arg)
> +{
> +    rtems_name             name = rtems_build_name(0, 0, 0, 0);
> +    rtems_id               id;
> +
> +    id = tcb->Object.id;
> +    if (rtems_capture_task_api (id) != OBJECTS_POSIX_API)
> +        rtems_object_get_classic_name (id, &name);
> +    if (rtems_capture_match_name_id (((rtems_capture_control *)arg)->name,
> +                ((rtems_capture_control *)arg)->id, name, id))
> +    {
> +        tcb->Capture.control = NULL;

Should this 'return true' to short-circuit the thread_iterate?

> +    }
> +    return false;
> +}
> +
>  /*
>   * This function checks if a new control structure matches
>   * the given task and sets the control if it does.
> @@ -699,6 +716,7 @@ rtems_capture_close (void)
>    {
>      rtems_capture_control* delete = control;
>      control = control->next;
> +    _Thread_Iterate (rtems_capture_uninitialize_control, (void *)delete);
>      free (delete);
>    }
>
> @@ -789,6 +807,7 @@ rtems_capture_flush (bool prime)
>    {
>      rtems_interrupt_lock_context lock_context_global;
>      uint32_t                     cpu;
> +    uint32_t*                    cpu_flags;
>
>      rtems_interrupt_lock_acquire (&capture_lock_global, &lock_context_global);
>
> @@ -800,16 +819,17 @@ rtems_capture_flush (bool prime)
>
>      _Thread_Iterate (rtems_capture_flush_tcb, NULL);
>
> -    if (prime)
> -      capture_flags_global &= ~(RTEMS_CAPTURE_TRIGGERED | RTEMS_CAPTURE_OVERFLOW);
> -    else
> -      capture_flags_global &= ~RTEMS_CAPTURE_OVERFLOW;
> +    if (prime) {
> +      capture_flags_global &= ~RTEMS_CAPTURE_TRIGGERED;
> +    }

I don't know what this (and following) change is doing.

>
>      for (cpu=0; cpu < rtems_scheduler_get_processor_maximum(); cpu++) {
>        RTEMS_INTERRUPT_LOCK_REFERENCE( lock, &(capture_lock_on_cpu( cpu )) )
>        rtems_interrupt_lock_context lock_context_per_cpu;
>
>        rtems_interrupt_lock_acquire (lock, &lock_context_per_cpu);
> +      cpu_flags = &(capture_flags_on_cpu (cpu));
> +      *cpu_flags &= ~RTEMS_CAPTURE_OVERFLOW;
>        capture_count_on_cpu(cpu) = 0;
>        if (capture_records_on_cpu(cpu).buffer)
>          rtems_capture_buffer_flush( &capture_records_on_cpu(cpu) );
> @@ -885,6 +905,8 @@ rtems_capture_watch_del (rtems_name name, rtems_id id)
>
>        *prev_control = control->next;
>
> +      _Thread_Iterate (rtems_capture_uninitialize_control, (void *)control);
> +
>        rtems_interrupt_lock_release (&capture_lock_global, &lock_context);
>
>        free (control);
> diff --git a/cpukit/libmisc/capture/capture_support.c b/cpukit/libmisc/capture/capture_support.c
> index 5b685479d7..fb4ccfe2d8 100644
> --- a/cpukit/libmisc/capture/capture_support.c
> +++ b/cpukit/libmisc/capture/capture_support.c
> @@ -247,7 +247,7 @@ rtems_capture_print_record_capture(int                         cpu,
>      {
>        fprintf(stdout,"%2i ", cpu);
>        rtems_capture_print_timestamp (rec->time);
> -      fprintf (stdout, " %12" PRId32 " ", (uint32_t) diff);
> +      fprintf (stdout, " %12" PRIu32 " ", (uint32_t) diff);
>        rtems_monitor_dump_id (rec->task_id);
>        if (name != NULL)
>        {
> @@ -439,7 +439,11 @@ rtems_capture_print_watch_list (void)
>      fprintf (stdout, " ");
>      rtems_monitor_dump_id (rtems_capture_control_id (control));
>      fprintf (stdout, " ");
> -    rtems_monitor_dump_name (rtems_capture_control_name (control));
> +    char name[5] = "????";

This should be declared at the start of the block/function.

> +    if (rtems_capture_control_name(control) != 0) {
> +        _Objects_Name_to_string((Objects_Name)rtems_capture_control_name (control), false, name, 5);
This line is too long, although there are other style problems
throughout this patch. Hopefully we'll have automation to help that.
Also, I'd prefer sizeof() or a const/define macro value instead of the
hard-coded 5.

> +    }
> +    fprintf(stdout, "%s", name);
>      flags = rtems_capture_control_flags (control);
>      fprintf (stdout, " %c%c ",
>               rtems_capture_watch_global_on () ? 'g' : '-',
> --
> 2.25.1
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel