[Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily

Peter Maydell posted 2 patches 5 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20181019174958.26616-1-peter.maydell@linaro.org
Test docker-clang@ubuntu passed
Test checkpatch passed
Test asan passed
Test docker-mingw@fedora failed
Test docker-quick@centos7 passed
linux-user/aarch64/cpu_loop.c    | 13 +++++--------
linux-user/alpha/cpu_loop.c      | 12 ++++--------
linux-user/arm/cpu_loop.c        | 16 ++++------------
linux-user/cris/cpu_loop.c       | 16 ++++------------
linux-user/hppa/cpu_loop.c       | 11 ++++-------
linux-user/i386/cpu_loop.c       | 16 ++++------------
linux-user/m68k/cpu_loop.c       | 16 ++++------------
linux-user/microblaze/cpu_loop.c | 16 ++++------------
linux-user/mips/cpu_loop.c       | 16 ++++------------
linux-user/nios2/cpu_loop.c      | 14 +++++---------
linux-user/openrisc/cpu_loop.c   | 11 ++++-------
linux-user/ppc/cpu_loop.c        | 15 +++++----------
linux-user/riscv/cpu_loop.c      |  2 +-
linux-user/s390x/cpu_loop.c      |  9 +++------
linux-user/sh4/cpu_loop.c        | 17 ++++-------------
linux-user/sparc/cpu_loop.c      | 16 ++++------------
linux-user/xtensa/cpu_loop.c     | 11 ++++-------
17 files changed, 67 insertions(+), 160 deletions(-)
[Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily
Posted by Peter Maydell 5 years, 6 months ago
This patchset fixes a minor bug in our handling of SIGTRAP
in linux-user.

The CPU main-loop routines for linux-user generally call
gdb_handlesig() when they're about to queue a SIGTRAP signal.  This
is wrong, because queue_signal() will cause us to pend a signal, and
process_pending_signals() will then call gdb_handlesig() itself.  So
the effect is that we notify gdb of the SIGTRAP, and then if gdb says
"OK, continue with signal X" we will incorrectly notify gdb of the
signal X as well.  We don't do this double-notify for anything else,
only SIGTRAP.

This bug only manifests if the user responds to the reported SIGTRAP
using "signal SIGFOO" rather than "continue"; since the latter is the
overwhelmingly common thing to do after a breakpoint most people
won't have hit this.

Patch 1 fixes this bug for every target except nios2, by
deleting the incorrect code.

Patch 2 fixes nios2 separately, because it was doing some odd
things with gdb_handlesig(). This also fixes in passing a Coverity
issue.

Tested with "make check-tcg", and with some by-hand stepping
around with an attached gdb. NB that the nios2 patch is only
compile tested as I don't have a nios2 linux-user environment
and check-tcg doesn't cover it.

thanks
-- PMM

Peter Maydell (2):
  linux-user: Don't call gdb_handlesig() before queue_signal()
  linux-user: Clean up nios2 main loop signal handling

 linux-user/aarch64/cpu_loop.c    | 13 +++++--------
 linux-user/alpha/cpu_loop.c      | 12 ++++--------
 linux-user/arm/cpu_loop.c        | 16 ++++------------
 linux-user/cris/cpu_loop.c       | 16 ++++------------
 linux-user/hppa/cpu_loop.c       | 11 ++++-------
 linux-user/i386/cpu_loop.c       | 16 ++++------------
 linux-user/m68k/cpu_loop.c       | 16 ++++------------
 linux-user/microblaze/cpu_loop.c | 16 ++++------------
 linux-user/mips/cpu_loop.c       | 16 ++++------------
 linux-user/nios2/cpu_loop.c      | 14 +++++---------
 linux-user/openrisc/cpu_loop.c   | 11 ++++-------
 linux-user/ppc/cpu_loop.c        | 15 +++++----------
 linux-user/riscv/cpu_loop.c      |  2 +-
 linux-user/s390x/cpu_loop.c      |  9 +++------
 linux-user/sh4/cpu_loop.c        | 17 ++++-------------
 linux-user/sparc/cpu_loop.c      | 16 ++++------------
 linux-user/xtensa/cpu_loop.c     | 11 ++++-------
 17 files changed, 67 insertions(+), 160 deletions(-)

-- 
2.19.1


Re: [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily
Posted by Richard Henderson 5 years, 6 months ago
On 10/19/18 10:49 AM, Peter Maydell wrote:
> This patchset fixes a minor bug in our handling of SIGTRAP
> in linux-user.
> 
> The CPU main-loop routines for linux-user generally call
> gdb_handlesig() when they're about to queue a SIGTRAP signal.  This
> is wrong, because queue_signal() will cause us to pend a signal, and
> process_pending_signals() will then call gdb_handlesig() itself.  So
> the effect is that we notify gdb of the SIGTRAP, and then if gdb says
> "OK, continue with signal X" we will incorrectly notify gdb of the
> signal X as well.  We don't do this double-notify for anything else,
> only SIGTRAP.
> 
> This bug only manifests if the user responds to the reported SIGTRAP
> using "signal SIGFOO" rather than "continue"; since the latter is the
> overwhelmingly common thing to do after a breakpoint most people
> won't have hit this.
> 
> Patch 1 fixes this bug for every target except nios2, by
> deleting the incorrect code.
> 
> Patch 2 fixes nios2 separately, because it was doing some odd
> things with gdb_handlesig(). This also fixes in passing a Coverity
> issue.
> 
> Tested with "make check-tcg", and with some by-hand stepping
> around with an attached gdb. NB that the nios2 patch is only
> compile tested as I don't have a nios2 linux-user environment
> and check-tcg doesn't cover it.
> 
> thanks
> -- PMM
> 
> Peter Maydell (2):
>   linux-user: Don't call gdb_handlesig() before queue_signal()
>   linux-user: Clean up nios2 main loop signal handling

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily
Posted by Peter Maydell 5 years, 5 months ago
Ping? This got reviewed but hasn't made it to master...

thanks
-- PMM

On 19 October 2018 at 18:49, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset fixes a minor bug in our handling of SIGTRAP
> in linux-user.
>
> The CPU main-loop routines for linux-user generally call
> gdb_handlesig() when they're about to queue a SIGTRAP signal.  This
> is wrong, because queue_signal() will cause us to pend a signal, and
> process_pending_signals() will then call gdb_handlesig() itself.  So
> the effect is that we notify gdb of the SIGTRAP, and then if gdb says
> "OK, continue with signal X" we will incorrectly notify gdb of the
> signal X as well.  We don't do this double-notify for anything else,
> only SIGTRAP.
>
> This bug only manifests if the user responds to the reported SIGTRAP
> using "signal SIGFOO" rather than "continue"; since the latter is the
> overwhelmingly common thing to do after a breakpoint most people
> won't have hit this.
>
> Patch 1 fixes this bug for every target except nios2, by
> deleting the incorrect code.
>
> Patch 2 fixes nios2 separately, because it was doing some odd
> things with gdb_handlesig(). This also fixes in passing a Coverity
> issue.
>
> Tested with "make check-tcg", and with some by-hand stepping
> around with an attached gdb. NB that the nios2 patch is only
> compile tested as I don't have a nios2 linux-user environment
> and check-tcg doesn't cover it.
>
> thanks
> -- PMM
>
> Peter Maydell (2):
>   linux-user: Don't call gdb_handlesig() before queue_signal()
>   linux-user: Clean up nios2 main loop signal handling
>
>  linux-user/aarch64/cpu_loop.c    | 13 +++++--------
>  linux-user/alpha/cpu_loop.c      | 12 ++++--------
>  linux-user/arm/cpu_loop.c        | 16 ++++------------
>  linux-user/cris/cpu_loop.c       | 16 ++++------------
>  linux-user/hppa/cpu_loop.c       | 11 ++++-------
>  linux-user/i386/cpu_loop.c       | 16 ++++------------
>  linux-user/m68k/cpu_loop.c       | 16 ++++------------
>  linux-user/microblaze/cpu_loop.c | 16 ++++------------
>  linux-user/mips/cpu_loop.c       | 16 ++++------------
>  linux-user/nios2/cpu_loop.c      | 14 +++++---------
>  linux-user/openrisc/cpu_loop.c   | 11 ++++-------
>  linux-user/ppc/cpu_loop.c        | 15 +++++----------
>  linux-user/riscv/cpu_loop.c      |  2 +-
>  linux-user/s390x/cpu_loop.c      |  9 +++------
>  linux-user/sh4/cpu_loop.c        | 17 ++++-------------
>  linux-user/sparc/cpu_loop.c      | 16 ++++------------
>  linux-user/xtensa/cpu_loop.c     | 11 ++++-------
>  17 files changed, 67 insertions(+), 160 deletions(-)

Re: [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily
Posted by Laurent Vivier 5 years, 5 months ago
On 12/11/2018 15:39, Peter Maydell wrote:
> Ping? This got reviewed but hasn't made it to master...

Do you want it in 3.1 too?

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 0/2] linux-user: Don't call gdb_handlesig unnecessarily
Posted by Peter Maydell 5 years, 5 months ago
On 12 November 2018 at 14:46, Laurent Vivier <laurent@vivier.eu> wrote:
> On 12/11/2018 15:39, Peter Maydell wrote:
>> Ping? This got reviewed but hasn't made it to master...
>
> Do you want it in 3.1 too?

Well, it is a bugfix, and we haven't rolled rc1 yet, so I would
default to yes, unless you'd rather defer it to 4.0.

thanks
-- PMM