From owner-svn-src-all@freebsd.org Sat Mar 10 06:18:19 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 07083F276D2; Sat, 10 Mar 2018 06:18:19 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 4D05A85724; Sat, 10 Mar 2018 06:18:17 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id E1B0D1049B04; Sat, 10 Mar 2018 16:56:18 +1100 (AEDT) Date: Sat, 10 Mar 2018 16:56:17 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: John Baldwin cc: Andriy Gapon , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r330338 - head/sys/amd64/amd64 In-Reply-To: <1997852.rATkjh2guz@ralph.baldwin.cx> Message-ID: <20180310154454.D971@besplex.bde.org> References: <201803031510.w23FAbeC065867@repo.freebsd.org> <2557369.6nFzd3kAUm@ralph.baldwin.cx> <20180310070452.P5919@besplex.bde.org> <1997852.rATkjh2guz@ralph.baldwin.cx> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=cIaQihWN c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=6I5d2MoRAAAA:8 a=hNusopO0UmrS9mlNGgQA:9 a=5wjp8Tm52k4UfIQL:21 a=R6By-DyKjgHlol7m:21 a=VGjOnp0SFQuy57pt:21 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 10 Mar 2018 06:18:19 -0000 On Fri, 9 Mar 2018, John Baldwin wrote: > On Saturday, March 10, 2018 07:41:30 AM Bruce Evans wrote: >> On Fri, 9 Mar 2018, John Baldwin wrote: >> >>> On Saturday, March 03, 2018 03:10:37 PM Andriy Gapon wrote: >>>> Author: avg >>>> Date: Sat Mar 3 15:10:37 2018 >>>> New Revision: 330338 >>>> URL: https://svnweb.freebsd.org/changeset/base/330338 >>>> >>>> Log: >>>> db_nextframe/amd64: catch up with r328083 to recognize fast_syscall_common >>>> >>>> Since that change the system call stack traces look like this: >>>> ... >>>> sys___sysctl() at sys___sysctl+0x5f/frame 0xfffffe0028e13ac0 >>>> amd64_syscall() at amd64_syscall+0x79b/frame 0xfffffe0028e13bf0 >>>> fast_syscall_common() at fast_syscall_common+0x101/frame 0xfffffe0028e13bf0 >>>> So, db_nextframe() stopped recognizing the system call frame. >>>> This commit should fix that. >>>> >>>> Reviewed by: kib >>>> MFC after: 4 days >>>> >>>> Modified: >>>> head/sys/amd64/amd64/db_trace.c >>>> >>>> Modified: head/sys/amd64/amd64/db_trace.c >>>> ============================================================================== >>>> --- head/sys/amd64/amd64/db_trace.c Sat Mar 3 13:20:44 2018 (r330337) >>>> +++ head/sys/amd64/amd64/db_trace.c Sat Mar 3 15:10:37 2018 (r330338) >>>> @@ -212,7 +212,9 @@ db_nextframe(struct amd64_frame **fp, db_addr_t *ip, s >>>> strcmp(name, "Xcpususpend") == 0 || >>>> strcmp(name, "Xrendezvous") == 0) >>>> frame_type = INTERRUPT; >>>> - else if (strcmp(name, "Xfast_syscall") == 0) >>>> + else if (strcmp(name, "Xfast_syscall") == 0 || >>>> + strcmp(name, "Xfast_syscall_pti") == 0 || >>>> + strcmp(name, "fast_syscall_common") == 0) >>>> frame_type = SYSCALL; >>> >>> I think you actually just want to replace Xfast_syscall with >>> fast_syscall_common. Neither Xfast_syscall nor Xfast_syscall_pti call any >>> functions before jumping to the common label, so when unwinding from a system >>> call you should always get the common label. (That is, I think we should >>> remove Xfast_syscall and Xfast_syscall_pti here. Any stack trace that >>> happens to find those symbols during unwinding won't have a valid SYSCALL >>> frame to unwind.) >> >> No, it needs these symbols to decode the frame after reaching a point where >> the frame is actually set up. > > My point is that during the instructions from Xfast_syscall to fast_syscall_common > there isn't a valid frame. I know. > Xfast_syscall has two instructions and hasn't yet > decremented %rsp to create room for the trapframe for example. If you wanted to > handle the special case of stepping through those functions you'd have to create > a new type of frame that used register values from the saved frame for the > debug trap. You can't assume that there's a 'struct trapframe' at 'rbp + 16'. In my version (only written for i386 so far), the strcmp()s are not even reached for syscalls since t is known and discovered that there is no trapframe at [er]bp + 16 (since that is not in the kernel stack). For nested exceptions, [er]bp + 16 is on the kernel stack and the trap printing for the reentry is bogus before the frame is set up, but ebp is now a usually-good frame for the interrupted context and the backtrace continues (as in -current) back to the top frame with no further problems except for a wrong name at the top level caused by cross-jumping. >> Also, in uncommitted fixes I add some decoding of the non-frame between >> the entry point and when the frame is set up. Then the frame register >> is still the user's for the syscall case, so should not even be accessed. >> The i386 output looks like this: >> >> current: >> XX12: Breakpoint at Xint0x80_syscall: pushl $0x2 >> XX12: db> t >> XX12: Tracing pid 1 tid 100001 td 0xc6fad000 >> XX12: Xint0x80_syscall(33,282,bfbfee0c,3b,0,...) at Xint0x80_syscall/frame 0xd1d05bd8 >> XX12: kernload(2,bfbfeec4,bfbfeed0,804812b,80a3d64,...) at 0x805188f/frame 0xbfbfee7c >> >> fixed: >> XX12F: Breakpoint at Xint0x80_syscall: pushl $0x2 >> XX12F: db> t >> XX12F: Tracing pid 1 tid 100001 td 0xd4c23000 >> XX12F: Xint0x80_syscall(2,bfbfeec4,bfbfeed0,804812b,80a3d64,...) at Xint0x80_syscall/frame 0xbfbfee7c >> XX12F: --- unknown trap, ebp = 0xbfbfee7c, sp = 0xd3399bdc, ks = 0xd3398000, kse = 0xd339a000 --- >> >> where most of the values on the last line are for debugging (ebp is the user >> stack pointer which will become inaccessible, sp is the local variable sp >> and [ks, kse - 1] is the thread's kernel stack range (sp must be in this >> else the backtrace stops). > > Yes, both of these symbols would only be found instructions for this type of > special frame. Using the 'SYSCALL' frame type for the Xfast_syscall* symbols > would always be wrong. Until such time as we have the new frame type we should > just ignore them. It's to have to have a new frame type. Setting up the frame takes 20-50 or more instructions and there is a different frame type at every instruction, possibly multiplied by different setups for different entries (similarly for doreti except it clearly has only one exit). (i386 is still optimized for the original i386 or possibly 8088's so it sets up the frame using lots of pushls and the offsets from esp change at every step. amd64 reserves space for the frame, but the frame contains garbage intil it is filled in and the order is unlikely to be to fill in ebp first so that the frame is usable for tracing early.) It is the cross-jump targets like fast_syscall_common that should be ignorded. This is easy to do by obfuscating their names as 1: or .Lfoo. This hides the name of one of the entry points for Xfast_syscall* instead of both. Xfast_syscall is last, so it is Xfast_syscall_pti that is hidden. This is least worst. The difference is small unless you are debugging pti and then all hidings of names are equally bad. Cross-jumping also breaks mcounting. mcount() follows the [er]bp chain in an even simpler way than backtrace. It knows nothing of frames and uses a different buggy method of determining kernel (instruction pointer) addresses: it checks the kernel's low and high pc recorded in the data where backtrace use INKERNEL(). Both assume a single flat address space for the kernel and user. I fixed this for backtrace by checking stack addresses instead of instruction pointer addresses. mcount() looks up the instruction pointer at each step so currently finds the cross-jump target fast_syscall_common instead of either actual entry point. ALTENTRY() has complications to separate the entry points for function calls, and CROSSJUMP*() is supposed to be used for cross-jumps, but CROSSJUMP*() is no useable before the frame is set up. CROSSJUMP*() is hard to use. It is better to not use cross jumps. Most uses were for the idle loop and went away when that was moved to C. All uses went away, although i386 needs at least 1 and amd64 probably needs many. I fixed 1 on i386 in some trees, but couldn't use CROSSJMP*() since it isn't general/complicated enough. >> Jumps and labels with names inside functions complicate things. I think >> fast_syscall_common needs to be in the list too, and the many alltraps >> labels should have been there. This will be more useful with my fix. >> ... See my other reply. It said that alltraps indeed causes problems for backtraces. > gdb does depend on the names, and I was looking at this commit again to see if > I needed to update gdb. I thought I didn't, but now I see that gdb was depending > on the 'X' prefix for the old Xfast_syscall name and it now needs to check for > fast_syscall_common directly. The X checks are dangerously general, but show another problem: the names of all entry points and pseudo-entry points should begin with X so that all versions of kgdb can find them. calltrap is handled quite differently on amd64 and i386. On amd64, it is treated like a normal entry point while on i386 it is treated like a double fault. i386 also uses calltrap to detect the change of the frame arg of trap() from direct to indirect. amd64 has the same change, but kgdb doesn't seem to detect it. Detecting it using the same method would be difficult since args are hard to find on amd64. Cross-jumps can be avoided using function calls, but that would be a bit slower. This method is already essentially used for interrupts -- call something and then do not much cleanup before jumping to doreti. Optimizations to avoid this call are not attempted. Bruce