From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 79469 invoked by alias); 14 Jun 2018 23:01:30 -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 79336 invoked by uid 89); 14 Jun 2018 23:01:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.6 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy=obliged, resistance, fds, LOCATION X-HELO: mailsec119.isp.belgacom.be Received: from mailsec119.isp.belgacom.be (HELO mailsec119.isp.belgacom.be) (195.238.20.115) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 14 Jun 2018 23:01:24 +0000 X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A2AyBABY8yJb/+ApQFddHAEBAQQBAQoBA?= =?us-ascii?q?YMaLoFPEoQhiGONcjEBljQLKwGEQAKCSCI4FAECAQEBAQEBAgFsKII1JAGCTgE?= =?us-ascii?q?BAQMBIw8BIzMIAQIOCgICJgICOR4GAYUtDI4km0eCHIRcg2yBaIELiRU/hBuHd?= =?us-ascii?q?oJVApkOBwKBao0XjTcrkRaBWCGBUm1TgkSCSI4IPYE9CAwBj1oBAQ?= X-IPAS-Result: =?us-ascii?q?A2AyBABY8yJb/+ApQFddHAEBAQQBAQoBAYMaLoFPEoQhiGO?= =?us-ascii?q?NcjEBljQLKwGEQAKCSCI4FAECAQEBAQEBAgFsKII1JAGCTgEBAQMBIw8BIzMIA?= =?us-ascii?q?QIOCgICJgICOR4GAYUtDI4km0eCHIRcg2yBaIELiRU/hBuHdoJVApkOBwKBao0?= =?us-ascii?q?XjTcrkRaBWCGBUm1TgkSCSI4IPYE9CAwBj1oBAQ?= Received: from 224.41-64-87.adsl-dyn.isp.belgacom.be (HELO md) ([87.64.41.224]) by relay.skynet.be with ESMTP/TLS/AES256-GCM-SHA384; 15 Jun 2018 01:01:09 +0200 Message-ID: <1529017269.2365.11.camel@skynet.be> Subject: Re: [RFA_v2 2/8] Implement frame apply [all | COUNT | -COUNT] [FLAGS...] COMMAND. From: Philippe Waroquiers To: Pedro Alves , gdb-patches@sourceware.org Date: Thu, 14 Jun 2018 23:01:00 -0000 In-Reply-To: References: <20180605204905.30612-1-philippe.waroquiers@skynet.be> <20180605204905.30612-3-philippe.waroquiers@skynet.be> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-06/txt/msg00383.txt.bz2 On Wed, 2018-06-13 at 20:52 +0100, Pedro Alves wrote: > Implement frame apply [all | COUNT | -COUNT] [OPTION]... COMMAND. > > Same comments apply to the "thread apply" patch too, of course. > > > Also implement the command 'faas COMMAND', a shortcut for > > 'frame apply all -s COMMAND'. > > > > Note: the syntax of 'frame apply' to specify some innermost or outermost > > frames is similar to 'backtrace' command. > > An alternative could be to make 'frame apply' similar to > > 'thread apply'. This was not chosen as: > > * the typical use cases for frame apply are all/some innermost/some outermost > > frames. > > * a range based syntax would have obliged the user to use absolute numbers > > to specify the outermost N frames, which is cumbersone. > > Or an syntax different from the thread range would have been needed > > (e.g. -0-4 to specify the 5 outermost frames). > > So, making frame apply similar to backtrace is found better. > > Hmm, I played with this a bit now, AFAICT, this syntax forces the applicable > range to start at either the innermost or outermost? How does one e.g., apply > the command to frames 10 to 20 (because e.g., they are the frames that > are running some library code), when there exist frames 0-50? > > > > > Th new command 'frame apply' allows to apply a COMMAND to a number of frames, > > or to all frames. > > This stands out as a limitation to me. Yes, it allows to (only) do what backtrace allows, and so, similarly to backtrace, does not allow to 'cherry pick' frames (or range of frames). I initially tried to find a reasonable syntax to allow a range based selection of frames, but could not obtain anything intuitive to specify the outermost N frames. Assuming no better syntax is found, maybe we need to explain better in the user manual that 'frame apply' selection of frame is similar/compatible with backtrace, maybe something like: 'frame apply' uses the same convention as 'backtrace' to specify the frames to apply COMMAND on : N innermost frames, or N outermost frames, or all frames. Note that 'thread apply' uses a list of thread IDs or thread IDs range. I understand that the differences is not ideal, but IMO, there are inherent differences between thread and frames, making it difficult to use a common syntax for 'frame lists' and 'thread lists'. E.g. there is no concept of innermost/outermost thread list, while I think we need a syntax for this concept in 'frame apply'. So, in summary, would be fine for me to change 'frame apply' for a better syntax, but which better syntax allows to specify as intuitively as backtrace N outermost frames ? And if we have another syntax than backtrace, then it should be similar to thread apply THREADID ranges, otherwise that would be very confusing. => I do not see a syntax consistent with thread apply ID ranges or individual IDs, that would allow to specify N innermost or outermost frames. > > I played with this a bit, and I have to admit that I found the > -v / -q combinations, and relativeness of the "quietness/verbosity" > level, a bit unintuitive and confusing. > > E.g.,: > > (top-gdb) frame apply 2 echo > #0 0x00007ffff54e0c6b in __GI___poll (fds=0x1a343d0, nfds=4, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:29 > #1 0x00000000007083e4 in gdb_wait_for_event (block=1) at src/gdb/event-loop.c:771 > > (top-gdb) frame apply 2 -q echo > 0x00007ffff54e0c6b 29 return SYSCALL_CANCEL (poll, fds, nfds, timeout); > 0x00000000007083e4 771 num_found = poll (gdb_notifier.poll_fds, > (top-gdb) > > Here, "-q" didn't make the output really be quieter, only different. Yes, the verbosity just controls the choice of the value of frame.h enum print_what: enum print_what   {      /* Print only the source line, like in stepi.  */     SRC_LINE = -1,      /* Print only the location, i.e. level, address (sometimes)        function, args, file, line, line num.  */     LOCATION,     /* Print both of the above.  */     SRC_AND_LOC,      /* Print location only, but always include the address.  */     LOC_AND_ADDRESS    }; and that is not really a 'monotonically increasing function'. > > Consider also if we add a knob to tweak the default verbosity. > Like "set frame-apply-default-format N" or whatever. > With that, "frame apply -q" does a different thing depending on > the value of the default setting. > > So I'm wondering whether "frame apply -quiet/-source/-location/-etc." > options wouldn't be better. > > Or "frame apply -format [0-4]" and/or > "frame apply -format quiet/source/location/location". > > Maybe alternatively consider ditching the format ideas, and have > users combine "frame apply -q" with "list" if they want to see > sources. OK, I expect resistance to that idea. :-) Not too much resistance on my side to drop the format idea :). On a second thought, this verbosity looks all too 'over-engineered' : thread apply verbosity currently only allows to print nothing or a fixed set of information per thread backtrace prints a fixed information per frame frame apply allows to choose whatever in enum print_what with non intuitive verbosity control. => I suggest to then just have the -q option : by default, frame information is printed like backtrace, and if -q is given, no information is printed. And same for 'thread apply' : by default, it prints a fixed set of info. And -q means do not print it. (we for sure need at least the -q to allow only the thread/frame info to be printed for successful COMMAND). (again, in the future, we can if we want add other options to control what to print, if we really need this flexibility). So, what do you think about the (simpler) idea of just having -q for both thread apply and frame apply ? Philippe