Re: [Qemu-devel] [PATCH v2 2/7] iotests: exclude killed processes from running under Valgrind

[Kevin Wolf]

Am 17.06.2019 um 15:20 hat Roman Kagan geschrieben:
> On Mon, Jun 17, 2019 at 02:53:55PM +0200, Kevin Wolf wrote:
> > Am 17.06.2019 um 14:18 hat Roman Kagan geschrieben:
> > > On Mon, Jun 17, 2019 at 01:15:04PM +0200, Kevin Wolf wrote:
> > > > Am 11.06.2019 um 20:02 hat Andrey Shinkevich geschrieben:
> > > > > The Valgrind tool fails to manage its termination when QEMU raises the
> > > > > signal SIGKILL. Lets exclude such test cases from running under the
> > > > > Valgrind because there is no sense to check memory issues that way.
> > > > > 
> > > > > Signed-off-by: Andrey Shinkevich <address@hidden>
> > > > 
> > > > I don't fully understand the reasoning here. Most interesting memory
> > > > access errors happen before a process terminates. (I'm not talking about
> > > > leaks here, but use-after-free, buffer overflows, uninitialised memory
> > > > etc.)
> > > 
> > > Nothing of the above, and nothing in general, happens in the usermode
> > > process upon SIGKILL delivery.
> > 
> > My point is, the interesting part is what the program does before
> > SIGKILL happens. There is value in reporting memory errors as long as we
> > can, even if the final check doesn't happen because of SIGKILL.
> 
> Agreed in general, but here the testcases that include 'sigraise 9' only
> do simple operations before that which are covered elsewhere too.  So
> the extra effort on making valgrind work with these testcases arguably
> isn't worth the extra value to be gained.

Ok, fair enough.

> > > > However, I do see that running these test cases with -valgrind ends in a
> > > > hang because the valgrind process keeps hanging around as a zombie
> > > > process and the test case doesn't reap it. I'm not exactly sure why that
> > > > is, but it looks more like a problem with the parent process (i.e. the
> > > > bash script).
> > > 
> > > It rather looks like valgrind getting confused about what to do with
> > > raise(SIGKILL) in the multithreaded case.
> > 
> > Well, valgrind can't do anything with SIGKILL, obviously, because it's
> > killed immediately.
> 
> Right, but it can do whatever it wants with raise(SIGKILL).  I haven't
> looked at valgrind sources, but
> 
>   # strace -ff valgind qemu-io -c 'sigraise 9'
> 
> shows SIGKILL neither sent nor received by any thread; it just shows the
> main thread exit and the second thread getting stuck waiting on a futex.

Oh, I didn't see this! So there isn't even a real SIGKILL signal.

> > But maybe the kernel does get confused for some
> > reason. I get the main threads as a zombie, but a second is still
> > running. Sending SIGKILL to the second thread, too, makes the test case
> > complete successfully.
> > 
> > So I guess the main question is why the second thread isn't
> > automatically killed when the main thread receives SIGKILL.
> 
> I don't see any thread receive SIGKILL.  So I tend to think this is
> valgrind's bug/feature.
> 
> Anyway the problem is outside of QEMU, so I think we need to weigh the
> costs of investigating it and implementing a workaround with the
> potential benefit.

I'd suggest to file a bug against valgrind at least. And indeed just
disable valgrind here like this patch does.

Kevin