Re: [Valgrind-developers] [PATCH 2/2] support pidfd_{getfd, send_signal} on linux/amd64

[Mark W. <ma...@kl...>]

Hi Ali,

On Wed, Nov 15, 2023 at 01:56:45PM +0100, Mark Wielaard wrote:
> I had to fixup some odd linebreaks in this patch to get it to apply.
> I also just hooked up the other linux arches since these are shared
> syscalls. See the attached result.
> 
> It looks good for pidfd_getfd, but I have one question about
> pidfd_send_signal.

As discussed on irc I split this patch in two (see attached).
And pushed the first pidfd_getfd one since that seems good.
The second is what I still have a question about.

This way we make some progress and have a smaller patch to discuss.

Cheers,

Mark

> On Wed, 2023-10-18 at 12:55 +0000, Ali Polatel via Valgrind-developers
> wrote:
> > +PRE(sys_pidfd_send_signal)
> > +{
> > +   PRINT("sys_pidfd_send_signal ( %ld, %ld )", SARG1, SARG2);
> > +   PRE_REG_READ
> > 2(long, "pidfd_send_signal", vki_pid_t, pid, int, signal);
> > +   if (!ML_(client_signal_OK)(ARG2)) {
> > +      SET_STATUS_Failure( VKI_EINVAL );
> > +      return;
> > +   }
> > +
> > +   /* If we're sending SIGKILL, check to see if the target is one of
> > +      our threads and handle it specially. */
> > +   SET_STATUS_Success(0);
> > +
> > +   if (VG_(clo_trace_signals))
> > +      VG_(message)(Vg_DebugMsg, "kill: sent signal %ld to pid %ld\n",
> > +		   SARG2, SARG1);
> > +
> > +   /* This kill might have given us a pending signal.  Ask for a check once
> > +      the syscall is done. */
> > +   *flags |= SfPollAfter;
> > +}
> > 
> 
> So this works almost just like the generic sys_kill. Except instead of
> an pid_t we get an pidfd (int). So I assume the PRE_REG_READ2 should
> s/vki_pid_t, pid/int, pidfd/
> 
> Also the comment in the middle is correct:
> /* If we're sending SIGKILL, check to see if the target is one of
>    our threads and handle it specially. */
> But then you just unconditionally SET_STATUS_Success(0). Why?
> 
> Thanks,
> 
> Mark