From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 107882 invoked by alias); 12 Jan 2016 20:01:12 -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 107873 invoked by uid 89); 12 Jan 2016 20:01:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=great!, 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 20:01:10 +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 BB9A733BF3; Tue, 12 Jan 2016 20:01:09 +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 u0CK1862028151; Tue, 12 Jan 2016 15:01:08 -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> <56954F8C.6010100@redhat.com> <56955283.1060502@redhat.com> Cc: philippe.waroquiers@skynet.be, sergiodj@redhat.com, eliz@gnu.org, xdje42@gmail.com, scox@redhat.com From: Josh Stone Message-ID: <56955B84.7050905@redhat.com> Date: Tue, 12 Jan 2016 20:01: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: <56955283.1060502@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2016-01/txt/msg00238.txt.bz2 On 01/12/2016 11:22 AM, Pedro Alves wrote: > 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 first continue hits syscall_entry:execve. The second continue hits syscall_return:execve. The second is the bug I fixed -- if it forgot syscall_state across exec, the return was reported like another entry. After that, there might be an impending SIGTRAP stop, depending on whether PTRACE_O_TRACEEXEC is active. The step was a way of just effecting a SIGTRAP for everyone, so it was easy to match. >> 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. Great! I'll make these final tweaks and push it out.