[PATCH v4 3/4] Implement -e status=set option

[Paul Chaignon]

On Tue, Jun 18, 2019 at 02:50:27AM +0300, Dmitry V. Levin wrote:
> On Sat, Jun 15, 2019 at 07:32:45PM +0200, Paul Chaignon wrote:
> [...]
> > diff --git a/number_set.c b/number_set.c
> > index 4092ffda..878fad48 100644
> > --- a/number_set.c
> > +++ b/number_set.c
> > @@ -47,6 +47,31 @@ reallocate_number_set(struct number_set *const set, const unsigned int new_nslot
> >     set->nslots = new_nslots;
> >  }
> >
> > +static unsigned int
> > +popcount(const unsigned int x)
> > +{
> > +   unsigned int count = 0;
> > +
> > +#ifdef HAVE___BUILTIN_POPCOUNT
> > +   count = __builtin_popcount(x);
> > +#else
> > +   for (; x; ++count)
> > +           x &= x - 1;
> > +#endif
> > +   return count;
> > +}
>
> We already have something similar in signal.c,
> please avoid code duplication if possible.

Would util.c be a good place to move the popcount function common to
signal and number_set?

>
> > +static unsigned int
> > +get_number_setbit(const struct number_set *const set)
> > +{
> > +   unsigned int i;
> > +   unsigned int count = 0;
> > +
> > +   for (i = 0; i < set->nslots; ++i)
> > +           count += popcount(set->vec[i]);
> > +   return count;
> > +}
>
> If we could assume sizeof(number_slot_t) == sizeof(uint32_t),
> this would be equivalent to popcount32(set->vec, set->nslots).

Should I add a static_assert to ensure this equality remains valid?  If I
should, is it preferred to place the static_assert next to the code using
it (get_number_setbit) or next to the typedef?

>
> [...]
> > @@ -39,8 +42,18 @@ alloc_number_set_array(unsigned int nmemb) ATTRIBUTE_MALLOC;
> >  extern void
> >  free_number_set_array(struct number_set *, unsigned int nmemb);
> >
> > +enum status_t {
> > +   STATUS_SUCCESSFUL,
> > +   STATUS_FAILED,
> > +   STATUS_UNFINISHED,
> > +   STATUS_UNAVAILABLE,
> > +   STATUS_DETACHED,
> > +   __MAX_STATUS,
> > +};
>
> Identifiers that begin with an underscore are reserved,
> please think of a different name.

Ok.  I went through the other enums defined in the code.  I will follow
tests/threads-execve's format and rename to NUMBER_OF_STATUSES.

>
> [...]
> > +.TP
> > +.BR "\-e\ status" = unavailable
> > +Trace system calls that returned but strace failed to fetch the error status.
> > +.TP
> > +.BR "\-e\ status" = detached
> > +Trace system calls for which strace detached before the return.
> > +.TP
> >  \fB\-e\ inject\fR=\,\fIset\/\fR[:\fBerror\fR=\,\fIerrno\/\fR|:\fBretval\fR=\,\fIvalue\/\fR][:\fBsignal\fR=\,\fIsig\/\fR][:\fBsyscall\fR=\fIsyscall\fR][:\fBdelay_enter\fR=\,\fIusecs\/\fR][:\fBdelay_exit\fR=\,\fIusecs\/\fR][:\fBwhen\fR=\,\fIexpr\/\fR]
> >  Perform syscall tampering for the specified set of syscalls.
>
> Unfortunately, due to unusual formatting the whole description
> of status keyword looks ugly.

Hm.  I misunderstood how .B and .IR work and didn't check the output for
the last changes.  Sorry about that.

> [...]
> > @@ -760,13 +765,6 @@ syscall_exiting_trace(struct tcb *tcp, struct timespec *ts, int res)
> >     if (raw(tcp)) {
> >             /* sys_res = printargs(tcp); - but it's nop on sysexit */
> >     } else {
> > -           if ((not_failing_only && syserror(tcp)) ||
> > -               (failing_only && !syserror(tcp))) {
> > -                   strace_close_memstream(tcp, false);
> > -                   line_ended();
> > -                   return 0;       /* ignore failed/successful
> > -                                    * syscalls */
> > -           }
> >             if (tcp->sys_func_rval & RVAL_DECODED)
> >                     sys_res = tcp->sys_func_rval;
> >             else
>
> Interesting.  Is there any particular reason to remove this optimization?

No particular reason.  I'll add it back right after this if/else block.

>

>
> --
> ldv