From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29645 invoked by alias); 12 Jan 2016 19:10:09 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 29634 invoked by uid 89); 12 Jan 2016 19:10:08 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=delivers, commented, Space, verbatim X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 12 Jan 2016 19:10:07 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 20DDA3B712; Tue, 12 Jan 2016 19:10:06 +0000 (UTC) Received: from [10.3.113.208] (ovpn-113-208.phx2.redhat.com [10.3.113.208]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0CJA4hG029653; Tue, 12 Jan 2016 14:10:04 -0500 Subject: Re: [PATCH v4] Implement 'catch syscall' for gdbserver To: Pedro Alves , gdb-patches@sourceware.org References: <1449196006-13759-2-git-send-email-jistone@redhat.com> <1452308954-13679-1-git-send-email-jistone@redhat.com> <5694EC0E.2080904@redhat.com> Cc: philippe.waroquiers@skynet.be, sergiodj@redhat.com, eliz@gnu.org, xdje42@gmail.com, scox@redhat.com From: Josh Stone Message-ID: <56954F8C.6010100@redhat.com> Date: Tue, 12 Jan 2016 19:10:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: <5694EC0E.2080904@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-01/txt/msg00235.txt.bz2 On 01/12/2016 04:05 AM, Pedro Alves wrote: > On 01/09/2016 03:09 AM, Josh Stone wrote: > >> >> 2016-01-08 Josh Stone >> Philippe Waroquiers >> >> * gdb.texinfo (Remote Configuration): List the QCatchSyscalls packet. >> (Stop Reply Packets): List the syscall entry and return stop reasons. >> (General Query Packets): Describe QCatchSyscalls, and add it to the >> table and detailed list of stub features. >> > > "table of detailed", I think. I'm referring to two hunks: - the table: Feature Name / Value Required / Default / Probe Allowed - the list below it, "currently defined stub features, in more detail" Maybe I just need another article, "to the table and the detailed list" >> @@ -648,6 +658,12 @@ handle_extended_wait (struct lwp_info **orig_event_lwp, int wstat) >> event_thr->last_resume_kind = resume_continue; >> event_thr->last_status.kind = TARGET_WAITKIND_IGNORE; >> >> + /* Update syscall state in the new lwp, effectively mid-syscall too. >> + The client really should send a new list to catch, in case the >> + architecture changed, but for ANY_SYSCALL it doesn't matter. */ >> + event_lwp->syscall_state = TARGET_WAITKIND_SYSCALL_ENTRY; >> + proc->syscalls_to_catch = syscalls_to_catch; > > The tone of this comment sounds to me as if the client should always > send a new list, just in case, but for some odd reason it sometimes doesn't. > > I think we want to convey the opposite, like: > > /* Update syscall state in the new lwp, effectively mid-syscall too. */ > event_lwp->syscall_state = TARGET_WAITKIND_SYSCALL_ENTRY; > > /* Restore the list to catch. Don't rely on the client, which is free > to avoid sending a new list when the architecture doesn't change. > Also, for ANY_SYSCALL, the architecture doesn't really matter. */ > proc->syscalls_to_catch = syscalls_to_catch; Sure, I'll take your rewrite verbatim, if you don't mind. >> static int >> +linux_supports_catch_syscall (void) >> +{ >> + return (the_low_target.get_syscall_trapinfo != NULL >> + && linux_supports_tracesysgood()); > > Space: "linux_supports_tracesysgood ()" OK >> +proc test_catch_syscall_execve {} { >> + global gdb_prompt decimal >> + >> + with_test_prefix "execve" { >> + >> + # Tell the test program we want an execve. >> + gdb_test_no_output "set do_execve = 1" >> + >> + # Check for entry/return across the execve, making sure that the >> + # syscall_state isn't lost when turning into a new process. >> + insert_catch_syscall_with_arg "execve" >> + check_continue "execve" >> + >> + # Remotes that don't track exec may report the raw SIGTRAP for it. >> + # If we use stepi now, we'll get a consistent trap for all targets. >> + gdb_test "stepi" ".*" "step after execve" > > Why is it important to do this raw SIGTRAP handling? What happens if you don't > do this? Won't those targets already FAIL the check_continue tests? Just in case, the context from Linux man ptrace: If the PTRACE_O_TRACEEXEC option is not in effect for the execing tracee, and if the tracee was PTRACE_ATTACHed rather that PTRACE_SEIZEd, the kernel delivers an extra SIGTRAP to the tracee after execve(2) returns. This is an ordinary signal (similar to one which can be generated by kill -TRAP), not a special kind of ptrace-stop. Since that's a signal-stop *after* execve returns, the check_continue will have succeeded already. The check_continue is really the only bit I care about for this test anyway. The rest is just trying to finish the target process cleanly. I was having trouble matching consistent output since plain remote was getting that SIGTRAP, but extended-remote would use exec events and not report anything extra. Adding the stepi made both stop the same way. This is moot now, since plain remotes are now tracking exec events too. I developed this test just before that went in last month. :) I just tried with that stepi commented out, and the test still passes on local, remote, and extended-remote, so I'll remove it.