From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 42690 invoked by alias); 12 Jan 2016 19:22:49 -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 42671 invoked by uid 89); 12 Jan 2016 19:22:48 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 spammy=H*M:1060502, month, Required 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:22:47 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 2ED8418B320; Tue, 12 Jan 2016 19:22:46 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0CJMhPP022129; Tue, 12 Jan 2016 14:22:44 -0500 Message-ID: <56955283.1060502@redhat.com> Date: Tue, 12 Jan 2016 19:22:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Josh Stone , gdb-patches@sourceware.org CC: philippe.waroquiers@skynet.be, sergiodj@redhat.com, eliz@gnu.org, xdje42@gmail.com, scox@redhat.com Subject: Re: [PATCH v4] Implement 'catch syscall' for gdbserver References: <1449196006-13759-2-git-send-email-jistone@redhat.com> <1452308954-13679-1-git-send-email-jistone@redhat.com> <5694EC0E.2080904@redhat.com> <56954F8C.6010100@redhat.com> In-Reply-To: <56954F8C.6010100@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-01/txt/msg00236.txt.bz2 On 01/12/2016 07:10 PM, Josh Stone wrote: > 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" Ah. Yes, that way I think wouldn't have been confused. > >>> @@ -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. Certainly 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. Still can't see how that step would help -- check_continue does two "continue"s. So one would stop at the random SIGTRAP, and FAIL, and another would lose control of the inferior, probably running to end. > > 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. OK, yes, let's drop it then. :-) Patch is OK with these changes, BTW. Thanks, Pedro Alves