Message ID | 20170210163650.10334-1-simon.marchi@ericsson.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 51709 invoked by alias); 10 Feb 2017 16:37:20 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 50638 invoked by uid 89); 10 Feb 2017 16:37:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 spammy=answered, onward, pipes, streams X-HELO: sesbmg23.ericsson.net Received: from sesbmg23.ericsson.net (HELO sesbmg23.ericsson.net) (193.180.251.37) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 10 Feb 2017 16:37:09 +0000 Received: from ESESSHC020.ericsson.se (Unknown_Domain [153.88.183.78]) by (Symantec Mail Security) with SMTP id C5.21.27784.03CED985; Fri, 10 Feb 2017 17:37:07 +0100 (CET) Received: from EUR03-DB5-obe.outbound.protection.outlook.com (153.88.183.145) by oa.msg.ericsson.com (153.88.183.78) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 10 Feb 2017 17:37:04 +0100 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=simon.marchi@ericsson.com; Received: from elxcz23q12-y4.ca.am.ericsson.se (192.75.88.130) by AMSPR07MB392.eurprd07.prod.outlook.com (10.242.22.21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P384) id 15.1.888.5; Fri, 10 Feb 2017 16:37:02 +0000 From: Simon Marchi <simon.marchi@ericsson.com> To: <gdb-patches@sourceware.org> CC: Simon Marchi <simon.marchi@ericsson.com> Subject: [PATCH] Don't send queries to the MI interpreter Date: Fri, 10 Feb 2017 11:36:50 -0500 Message-ID: <20170210163650.10334-1-simon.marchi@ericsson.com> MIME-Version: 1.0 Content-Type: text/plain X-ClientProxiedBy: BN6PR14CA0016.namprd14.prod.outlook.com (10.173.157.154) To AMSPR07MB392.eurprd07.prod.outlook.com (10.242.22.21) X-MS-Office365-Filtering-Correlation-Id: c94dcf1c-d2da-4be7-29a6-08d451d30b2d X-Microsoft-Antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001); SRVR:AMSPR07MB392; X-Microsoft-Exchange-Diagnostics: 1; AMSPR07MB392; 3:PubZCKQnh/eSYV5F4HgVMzIvmFSqQEr0ADi8+cX2oAxz0JydrwlLz82T5Y3Nw1H/RUQCHCVMrX6B3He6vnano3/KQUxwo9Ra2X8korjUzazdVXrplhaOF9SHn6TH+as0o28jGvYeF5tTy7+d+1N4leWZpHRpwdxnYiYDi44BiTFAdcT8LYKuMM25milw7YLMG9UmLVcYS/NMuj8zodRev6OQOc0DCb0kLkk2ZIRp1TdTGc2nUqb72qJROH9nvvrMvuawx1aewsEs3dh8bgzfAQ==; 25:MS3j8VRQgxKgmmL5zCNlfDi5U4FYwigHBieAwEFTaz2MGClNpP/rEGHhYw+diDCagKY/TcEuIovtkNMb/3rn+EHdkL0Z53OV9qYVZec/YHSwEoAsz3arHmIWwqJzqk2zDlVEanOzLOv9HcW/Ew596F1dU0AgTTGxKeQLDdi723HV1VgHX38qVpXyYnNb0sG9h1Wd1AXMIGDtJFESU7AIK+CCuKEXKncdmK060ja2iPGq07Xs8ni/Suhy6TNPHpwCWVmbMKeNGpDClN/0xpM4f77on6FpvPLNv/t/oJ7hLjhGgasY/QI+Aemb8rTnNd3Jbkhy/lkVPOBjlIoUOENyV3XPg3NX3jHmb6Hrp6qVpmsQDZTh2nwuZt83IUDVWeUNxvo/7yiKzYGpO8ux+srO2ItxDgN38PYwYpmhxIro6YmK7wiAzHLNkQ1Yrg1LE171qzIoFyTxxT6pXktDrKXHVg== X-Microsoft-Exchange-Diagnostics: 1; AMSPR07MB392; 31:0FscNBYodrPw0RwVwrh1ANewr9saSgbwQ1etN0wrHW3NqO/nLrNmIExUASQR2Yrzv6nlobksAJJaotY2cEj/fGNQT8JTVieFMCcefmcwCyFIYVOqx2ZpVw4ocBtt7aSwlzV3xGcfqlRMkb/BUM34BIcxqYHW0DAmyTG5IlIgNxzwDDekaY0C+/ifK7OfcfiAtfSHvWHEZ8nhm1pYbBBmV/EoBprMJAn0B+kSDMm00IfEIfuNw7eNx38USekn0n/8; 20:1u65Tts7YhljLlocp9sv+GnE3wGRAYr96VdaXxjEd6oHj7ivXy40CNaoBSDKSrH/8SHNvSyHXhkUnWRjI2szUfpsRPL+TMd78LxNQ6zEIta8Xz+ocbEFVukU4CsY+pbivWO6JLW0IAejKggbwQR6zf89Z0Qfg1Kf+xQQtq7WR3hgFsnu4Sc0ElZTq//78foAywskR778smd4x9dNE3jlGiPcoI8iDMl4eQdhfgwCxr0OIZncKTcdNfZPlDPzEYch/UqixJtkj9zUUQDISqgfC+b2aJWWzjD+WebmmYhV3OQG3VnBxpmYisGLvMz5OxHFT2iUCMQvFrDDBFeDdHzlLOG7MP0xSAUKBAg/CADPbgLS5/vS+qlC0ZB5abQXZ8PK0z1maJJH+te4VBnHEEMEkKa+3akdaOv9CQt5YAOwpEqewFwZZ8BeHBclIHa0KYq0GADtU/YGrxDZU+XZn4JkhOAO+aGTJZ2n1cRRwg6eKycJcQ08IeTllyW/tiF8HPzt X-Microsoft-Antispam-PRVS: <AMSPR07MB3925044CDCD7BC0C36E2D8BED440@AMSPR07MB392.eurprd07.prod.outlook.com> X-Exchange-Antispam-Report-Test: UriScan:(158342451672863); X-Exchange-Antispam-Report-CFA-Test: BCL:0; PCL:0; RULEID:(6040375)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6041248)(20161123562025)(20161123560025)(20161123555025)(20161123558025)(20161123564025)(6072148); SRVR:AMSPR07MB392; BCL:0; PCL:0; RULEID:; SRVR:AMSPR07MB392; X-Microsoft-Exchange-Diagnostics: 1; AMSPR07MB392; 4:KmM1afqWU6xZBYbHbQF9P3I5UK4JSwQR3tsK9fsKwYhhQ99DqyeM1WJcuOrryX/INtneuxnxYgGmDGubi492N2BB48dB8Aqd6bwhiqANr7obIjlYqIB12GoHdOVu2444bgNroMZyykr74GZKQX3Zdt5EcQPa9LEb9MT20+FPv+uGyaSdOHuLGt7k+Al1RlOz9V2CxaBqVAFxy8/bzdU+KdhumQ4Iwta3ZoR3RKN19nkG/+2n/3NAoNVae3kHvIdzpuUyhidNnHV8aLdSXW8yTqKu9jTNnI0j4i3K7SunG9YVFjpE8TXm7CjV3lua9+5UCkcsNVKZnmxKY+lqzi05d+KabFvp/O0CpmD926kpRHkaNxLwoGUAW1LD0ALP2U1++N292HKtKsWYMyORCdNMrMESDwtjOARNLuPrrYdEl8AD6KynLtrxDNOgFS2OciRsSFQHLrwdhfqBEXH4i1ozpZ9hMwQmviS+drr32xEUoX7+tw0yUAwn7ESHxUNiCRIwh31C4rBJ6xoQSL5lNeyhoMq6P7HvIIPgpFNcZgjJf2W73lKjq8zb547J0Nkp+kMcLoo8JIcq6re3xJ6Ia9Tuxc8MCGvLbnXNSKYBOnjZScDzOTQVuWkHShYAa+QLVQ9g233bcNkPtgSeBx/WhFtvDA== X-Forefront-PRVS: 0214EB3F68 X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(4630300001)(6009001)(7916002)(39450400003)(199003)(52314003)(54534003)(189002)(2906002)(189998001)(2351001)(4326007)(5003940100001)(8676002)(66066001)(50226002)(33646002)(81166006)(81156014)(101416001)(305945005)(7736002)(97736004)(50466002)(48376002)(53936002)(92566002)(47776003)(1076002)(450100001)(106356001)(105586002)(3846002)(6116002)(42186005)(6506006)(6486002)(50986999)(5660300001)(86362001)(25786008)(6666003)(36756003)(38730400002)(110136004)(6512007)(68736007)(6916009)(107886003); DIR:OUT; SFP:1101; SCL:1; SRVR:AMSPR07MB392; H:elxcz23q12-y4.ca.am.ericsson.se; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; Received-SPF: None (protection.outlook.com: ericsson.com does not designate permitted sender hosts) X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1; AMSPR07MB392; 23:Rytw7Rm7g4EBpdSByx2OgOssA/cObia+J4GpIIMp+5?= =?us-ascii?Q?B5HTqCt2fH5cyPp4zx/zJ8h5oA6qox1W1sa+oJYJotBexzFsd4CBoJk7rW7x?= =?us-ascii?Q?BB4nljGB016AP7yHOudAoLNpGlvj3w+xc4wd0CyFOH2Oo2StXitdVbtMkwkn?= =?us-ascii?Q?a8/dPfeOrZZPJ5PTRL2YM9atz0dZArIUz5/C9tt5Pnv3BDhq+6D2P5AQe+RT?= =?us-ascii?Q?qCOuicYvdASnuMUDXT9hNTR7YWxUDJinnRCOfsfpfMGwwTdprjlP154tuxNl?= =?us-ascii?Q?Lqg426EtNaVNfXa0UAvD2gUt9sgnF1D6XPi0QuQMj0k1borwqO2/IlIyWptV?= =?us-ascii?Q?OCS7+2sYejOy2REVRe6xNayt0ueNV3CJlgv90Jlt93XsxE/n4ovdweY6u1jU?= =?us-ascii?Q?Y3lL3nZ8/i6AfikdhfaUcjPUwSXeUa7zRRfxZv80lxazMSOPSTS2mUOGCD/w?= =?us-ascii?Q?aC5tIdzrgR6YYNV3ilD8Uzt/i9B5m5Wa+XcXARsWFdg1qzpH84QwCnO7BSAu?= =?us-ascii?Q?AeYxFpcs9p6uAE79w9YjFMOdRzqyGVuue7LZFNoXrRo1NTNIxBmkNMuOoEPz?= =?us-ascii?Q?OzGGXL8Om+0InbBQbtmOqmCyXLzT+B1OQnM1NehUuzLfsKwa6DHUyklyCInf?= =?us-ascii?Q?IuDeQapgVwCVLacpdrpjrcY1B5Vgy8yipWquRnaDfSB4KTZT5cv8lJ4Ymekg?= =?us-ascii?Q?1Ej/NGLX665Hn8/1cCsfcusGcBXJOEqIHvoQptn679J/S29WwW35PaqjQt1w?= =?us-ascii?Q?30EyI+yPI/8WdvHHeLoGTYLb+lu7RfZYEOHS5L7a++NeNdBm9g9Cs2PaS5OT?= =?us-ascii?Q?ok2yYfZPhwNUEVhapec20JDxnwHJFBTeNntEXg+WYPmuTtB+fMfcnxd+KczD?= =?us-ascii?Q?xshk+5SEg2VzKGh+g614GosOcAT+FZPI1948LfEcr4j5LAWIxeRayuJpbs9L?= =?us-ascii?Q?C1C0dyEV4O0gnNvLtUSpFDrzD9gyDC1zVGwhwMhoMb7RcTiYcwK7JzIiYeFb?= =?us-ascii?Q?UhKudLX5rX56VpervFuQdsO98dLeOhqpIon+PUaq0dOJMAsgmK9XzNmkfW2v?= =?us-ascii?Q?SwCdFvvjNeYdukE4FTYGNjuo/uJ/7hX1BLsJ9IhQgtAnMahl0bqfDZf5HQb2?= =?us-ascii?Q?SkM3iAkUCf0xMBOspl4CEIxhkzFlzFNCqB23O3KnC2MAbGsTOgezUyJK/LVq?= =?us-ascii?Q?+QyJ0cCDCmcsI=3D?= X-Microsoft-Exchange-Diagnostics: 1; AMSPR07MB392; 6:Zt+7soammCdR0yA1pJ5/pN7dkK5wA1VyHPhDUTZjhxC8uTTtVKFU4qtJjPmaRt7RNQGTtQppIPGU1ymCd9ZLX768HaGV+6G93WXQDxuMX4gD2UaWK1GyiSnSis9Hp9zXAnUWNzJVm/5u7JE/xJp4PPs1pKExfLgcLvV38P9s3EKXOobovqwKG8bN5RiDTO7fE+xByfEIqRM8lXxhPrLPN0Q4weqk21zMJ38/vKQ0R0lt0n8T1dQN4W5rM9DXSmoxGb2cYYLOWo7ihFysFJCArI+c7ayo/G2yYsSdCW5YZm8OZqrLcobgSHUEE5WL23Vut9lpUHu0hm3cEOMjqFf2E+2iOxyyOU0mFblHDF6ZUoWAJNDPiw0IQkbluFGMFFV3Zt38Ktpwya6zQKqTsR53RQ==; 5:FERRAQwIlDSR971fu1EqAVrkn0OKyqCXEg2ZPpcazW0ehTD6TrMEzEid1eGEl1g9DGuJyxCsGpf8CEaZqwFd/cQdOiUpNR3JSthZryk5htRh9AjPGKiplUWCiHxgxM0Cy6dQ4HiTddeAbjM332vUVg==; 24:6kH+Xvb4y3bWHk0yJe/05a54/nO58AfV3teB2D208Ae3vg0NfL08URzlrINUsrz+7Z0/DGTXEgHG2PAEJpMa5mBkAOylVeSMzHO7aXPoDQI= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1; AMSPR07MB392; 7:+ieYr3qoBexbt/ijq+CweqB1I423Bt/l0LCGgv9Tm98+WOC+4rO54Z3GQRII694w1UiB9S1Vto4nGpEX95QkaDyk1qR+Vq65Prvac4gNdhW4t/VYQu7ODZKo+cFfxDxQ9jOL2J5vy3zwdEXU6CKLhO31sororITD9WYyjtYLrjapSK9M8Gx6dVVozP3+q/tH08+u4I2kFAhxFjChJN0n160H2Db/8FV3PW4lgYgjk/3EqAnYsrq7OaNMMLDLAWaOUJZKxq/gfO+D1D87V8sanwb3YdSNHHDwd1b6wMTFdckbfMYFYGXzGKgY5NF8wZv9c/NU1pU+AU8ghny9nsgkssVdxfJnEldVLH3Y5rnfMhLXKK/NRSQHad1rx/1sKk8hUD6EMZ5f1JpXZ8vb15I3OusoYu8Y/g0QT8A0RZ+qT9XsQUG2N8FfwToeT5RjFjSiw1G0Yor9NusDrdV4M24nVmuMFO/lBiaF85e3ObMKcx/tBQYFTJbO8MWWWYEo37zS+XDGeoZgHvtG31IE7aLCDQ== X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Feb 2017 16:37:02.9165 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: AMSPR07MB392 X-OriginatorOrg: ericsson.com X-IsSubscribed: yes |
Commit Message
Simon Marchi
Feb. 10, 2017, 4:36 p.m. UTC
We have a little problem in Eclipse CDT related to queries being sent on the MI channel. GDB sends queries on the MI stream and waits for an answer (y or n), but since CDT will never answer, it causes a deadlock. Note that this is only a problem when using MI as a side-channel (new-ui) on a dedicated tty. It doesn't happen if GDB's input/output streams are pipes, for example. In that case, the queries are auto-answered as they should. The situation can be triggered by modifying a variable value while replaying. Roughly, the steps are: - Start recording (record-full) - Step forward a bit - Step backwards a bit - Change a variable using the Variables view in the UI Changing the variable uses the -var-assign command. Modifying the memory while replaying causes the following query to be sent: ~"Because GDB is in replay mode, writing to memory will make the execution \ log unusable from this point onward. Write memory at address \ 0x7fffffffdabc?(y or n) " In the past, the query would get auto-answered because GDB would consider the input as non-interactive. The criterion for this is whether the input stream is a tty (ISATTY). Now that we create the MI channel with new-ui and pass it a tty (/dev/pts/X), GDB considers the input as interactive, so the query is not auto answered. I have a hard time defining what should be the right criteria for when we should or shouldn't query. I think we'll have to take into account what is the originating interpreter, CLI or MI. A special case is when we receive CLI through MI (either -interpreter-exec console or typing directly CLI commands): it's possible that it's a human at the other end, typing in a field in the front-end, which then forwards the commands to GDB. In that case, we could want to query, since the user is there to respond. However, it would be very fragile, as the front-end should not send any command in the mean time, not reply anything else than "y" or "n" and not wrap the user response in -interpreter-exec... Currently, we never query as a result of CLI-in-MI commands anyway, because deprecated_query_hook is set. So I think we should just keep this behaviour: queries resulting from CLI-in-MI commands get auto-answered. Conveniently, when using CLI in MI, the current_interpreter stays MI (unlike when using MI in CLI), so it allows to differentiate a CLI command from a CLI-in-MI command easily. So the criteria I came up with in order to decide whether we should not auto answer are: the input should be interactive (ISATTY is true) _and_ the originating interpreter should support queries. In practice, it means the only time we are going to query is when it's a CLI command typed in a CLI UI coming from a tty. Currently, CLI supports queries and MI doesn't. However, we could imagine that some day GDB could send the query to the front-end through MI, so that it displays a nice popup. That's a whole separate topic though, since it would require changing the MI paradigm a bit. gdb/ChangeLog: * interps.h (interp::supports_queries): New. * cli-interp.h (cli_interp_base::supports_queries): New. * cli-interp.c (cli_interp_base::supports_queries): New. * utils.c (defaulted_query): Don't query if the current interpreter doesn't support queries. --- gdb/cli/cli-interp.c | 6 ++++++ gdb/cli/cli-interp.h | 1 + gdb/interps.h | 4 ++++ gdb/utils.c | 5 ++++- 4 files changed, 15 insertions(+), 1 deletion(-)
Comments
On 02/10/2017 04:36 PM, Simon Marchi wrote: > We have a little problem in Eclipse CDT related to queries being sent on > the MI channel. GDB sends queries on the MI stream and waits for an > answer (y or n), but since CDT will never answer, it causes a deadlock. > > Note that this is only a problem when using MI as a side-channel > (new-ui) on a dedicated tty. It doesn't happen if GDB's input/output > streams are pipes, for example. In that case, the queries are > auto-answered as they should. I think we could have a testsuite test for this, as the 'new-ui'-related testcases create a pty for the secondary MI channel ("separate-mi-tty")? Thanks, Pedro Alves
On 2017-02-10 11:48, Pedro Alves wrote: > On 02/10/2017 04:36 PM, Simon Marchi wrote: >> We have a little problem in Eclipse CDT related to queries being sent >> on >> the MI channel. GDB sends queries on the MI stream and waits for an >> answer (y or n), but since CDT will never answer, it causes a >> deadlock. >> >> Note that this is only a problem when using MI as a side-channel >> (new-ui) on a dedicated tty. It doesn't happen if GDB's input/output >> streams are pipes, for example. In that case, the queries are >> auto-answered as they should. > > I think we could have a testsuite test for this, as the > 'new-ui'-related > testcases create a pty for the secondary MI channel > ("separate-mi-tty")? Right, I didn't think of making a test, I'll work on it. I'll try to find a query that's easier to trigger than the modify-memory-while-replaying one though.
On 02/10/2017 04:52 PM, Simon Marchi wrote: > On 2017-02-10 11:48, Pedro Alves wrote: >> On 02/10/2017 04:36 PM, Simon Marchi wrote: >>> We have a little problem in Eclipse CDT related to queries being sent on >>> the MI channel. GDB sends queries on the MI stream and waits for an >>> answer (y or n), but since CDT will never answer, it causes a deadlock. >>> >>> Note that this is only a problem when using MI as a side-channel >>> (new-ui) on a dedicated tty. It doesn't happen if GDB's input/output >>> streams are pipes, for example. In that case, the queries are >>> auto-answered as they should. >> >> I think we could have a testsuite test for this, as the 'new-ui'-related >> testcases create a pty for the secondary MI channel ("separate-mi-tty")? > > Right, I didn't think of making a test, I'll work on it. I'll try to > find a query that's easier to trigger than the > modify-memory-while-replaying one though. I've run into these queries deep down in the record layer in the past, and wondered about getting rid of them. Particularly this one: if (!yquery (_("Do you want to auto delete previous execution " "log entries when record/replay buffer becomes " "full (record full stop-at-limit)?"))) error (_("Process record: stopped by user.")); In order to query, the inferior is already temporarily stopped. So why not just unconditionally error, and lets user tweak "set record stop-at-limit" and continue if they want. In this case, instead of: query (_("Because GDB is in replay mode, changing the " "value of a register will make the execution " "log unusable from this point onward. " "Change all registers?")); we'd error here with something like: Cannot change the value of a register in replay mode, it would make the execution log unusable from this point onward. See whatnot. and then if users really want to change memory, they would use something like a "record discard" or "record commit" or some such command that discards history leaving the inferior state as it is right now (not sure there's a command that does that already.) In scripts and maybe frontends, you'd use the existing "set record memory-query off", renamed to "set record allow-memory-write on" or some such. Thanks, Pedro Alves
On 02/10/2017 05:12 PM, Pedro Alves wrote: > I've run into these queries deep down in the record layer > in the past, and wondered about getting rid of them. > > Particularly this one: > > if (!yquery (_("Do you want to auto delete previous execution " > "log entries when record/replay buffer becomes " > "full (record full stop-at-limit)?"))) > error (_("Process record: stopped by user.")); > > In order to query, the inferior is already temporarily stopped. > So why not just unconditionally error, and lets user tweak > "set record stop-at-limit" and continue if they want. Let me expand on this a bit. I have a local unfinished branch somewhere that attempts to address the "pagination/queries in the primary CLI prevent MI async notifications (like *stopped) on the secondary MI channel" issue. I think there's a PR about it, but I can't find it now. It's the one that led to Eclipse disabling pagination. The issue is that pagination is handled by starting a new nested event loop deep inside the print that triggered the pagination, and while a secondary event loop is running, no target events are processed at all (see gdb_readline_wrapper, the target_async(0) call). So GDB will only notice that the inferior stopped for a trap when the pagination/query is unblocked. So the idea to address this was to continue processing target events in the background and send them to MI, even while the CLI is stopped in the pagination. Meanwhile, if some target event ends up producing _more_ output _while_ the CLI is already waiting for the user to request another page of output, we'd need to buffer the output somewhere temporarily, in order to later dump it on the screen when whatever we were outputting that paginated is fully flushed/output. This suggested actually always buffering asynchronous output and dumping it on the screen at a safe point. I.e., output generated inside handle_inferior_event would always go to a buffer, and then dumped on the screen after processing the event, when safe. And it is at this point that I thought that it is odd to query and ask for user input deep down inside the record layer, while handling some asynchronous execution event -- i.e., deep down inside handle_inferior_event. If we're buffering output, when the user won't see the query anyway. Hmm, now that I think of it, the "Do you want to auto delete previous execution" query wouldn't be converted to an error, but instead to a something like TARGET_WAITKIND_NO_HISTORY, I suppose. OK, I found the branch. Pushed here now: https://github.com/palves/gdb/commits/palves/console-pagination Thanks, Pedro Alves
On 02/10/2017 05:44 PM, Pedro Alves wrote: > OK, I found the branch. Pushed here now: > > https://github.com/palves/gdb/commits/palves/console-pagination And re-reading the branch again, I noticed this hunk: @@ -1255,7 +1258,9 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) way, important error messages don't get lost when talking to GDB over a pipe. */ if (current_ui->instream != current_ui->stdin_stream - || !input_interactive_p (current_ui)) + || !input_interactive_p (current_ui) + /* We can't handle nested queries. */ + || current_ui != main_ui) { old_chain = make_cleanup_restore_target_terminal (); @@ -2045,36 +2050,48 @@ begin_line (void) } } This is similar to your patch, but it handles something your version doesn't, I think. That is the case of handling the secondary channel being a CLI interpreter, not an MI one, and that UI having been started on a terminal. Until we put each UI/interpreter on its own thread, with its own event loop, we can't handle multiple UIs querying simultaneously. Picture this situation: Do this first on UI #1: (gdb) handle SIGINT SIGINT is used by the debugger. Are you sure you want to change it? (y or n) And then this on UI #2: (gdb) handle SIGTRAP SIGTRAP is used by the debugger. Are you sure you want to change it? (y or n) Now answer "yes" on UI #1. What happens? GDB incorrectly changes SIGTRAP, not SIGINT, and probably gets the UIs messed up. Why? Because we'd have this in pseudo-backtrace: #0 gdb_readline_wrapper #1 defaulted_query // for UI #2 #2 handle_command #3 execute_command ("handle SIGTRAP" .... #4 stdin_event_handler // input on UI #2 #5 gdb_do_one_event #7 gdb_readline_wrapper #8 defaulted_query // for UI #1 #9 handle_command #10 execute_command ("handle SIGINT" .... #11 stdin_event_handler // input on UI #1 #12 gdb_do_one_event #13 gdb_readline_wrapper So answering "yes", returns the read input to the query in frame #1... So that hunk addresses this by only allowing queries on the main UI, similarly to how we only enable readline on the main UI. So I think that to support multiple queries like that the simplest / most natural would be to make each UI above run on its own thread, so that each would have its own independent stack/frames. Thanks, Pedro Alves
On 02/10/2017 06:07 PM, Pedro Alves wrote: > On 02/10/2017 05:44 PM, Pedro Alves wrote: > >> OK, I found the branch. Pushed here now: >> >> https://github.com/palves/gdb/commits/palves/console-pagination > > And re-reading the branch again, I noticed this hunk: > > @@ -1255,7 +1258,9 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) > way, important error messages don't get lost when talking to GDB > over a pipe. */ > if (current_ui->instream != current_ui->stdin_stream > - || !input_interactive_p (current_ui)) > + || !input_interactive_p (current_ui) > + /* We can't handle nested queries. */ > + || current_ui != main_ui) > { > old_chain = make_cleanup_restore_target_terminal (); > > @@ -2045,36 +2050,48 @@ begin_line (void) > } > } > > This is similar to your patch, but it handles something your > version doesn't, I think. That is the case of handling the secondary > channel being a CLI interpreter, not an MI one, and that UI > having been started on a terminal. So with with all the rationale I've thrown out, and idea that we shouldn't see a query anyway, and considering that real MI commands shouldn't really query anyway (nobody sees the query output), it may be actually impossible to find a way to this with MI on the secondary channel, that will remain stable going forward. Hmm. So how about using the hunk shown above, which should handle your case as well, and while at it write a test that uses CLI in the secondary UI instead, making sure a query is auto-answered in that UI? Thanks, Pedro Alves
On 17-02-10 12:44 PM, Pedro Alves wrote: > And it is at this point that I thought that it is odd to > query and ask for user input deep down inside the record layer, > while handling some asynchronous execution event -- i.e., > deep down inside handle_inferior_event. If we're buffering > output, when the user won't see the query anyway. > Hmm, now that I think of it, the "Do you want to auto delete > previous execution" query wouldn't be converted to an error, > but instead to a something like TARGET_WAITKIND_NO_HISTORY, > I suppose. And TARGET_WAITKIND_NO_HISTORY would cause a user-visible stop? (I'm processing the rest and will reply to your latest message)
On 17-02-10 01:07 PM, Pedro Alves wrote: > On 02/10/2017 05:44 PM, Pedro Alves wrote: > >> OK, I found the branch. Pushed here now: >> >> https://github.com/palves/gdb/commits/palves/console-pagination > > And re-reading the branch again, I noticed this hunk: > > @@ -1255,7 +1258,9 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) > way, important error messages don't get lost when talking to GDB > over a pipe. */ > if (current_ui->instream != current_ui->stdin_stream > - || !input_interactive_p (current_ui)) > + || !input_interactive_p (current_ui) > + /* We can't handle nested queries. */ > + || current_ui != main_ui) > { > old_chain = make_cleanup_restore_target_terminal (); > > @@ -2045,36 +2050,48 @@ begin_line (void) > } > } > > This is similar to your patch, but it handles something your > version doesn't, I think. That is the case of handling the secondary > channel being a CLI interpreter, not an MI one, and that UI > having been started on a terminal. > > Until we put each UI/interpreter on its own thread, with its > own event loop, we can't handle multiple UIs querying > simultaneously. Picture this situation: > > Do this first on UI #1: > > (gdb) handle SIGINT > SIGINT is used by the debugger. > Are you sure you want to change it? (y or n) > > And then this on UI #2: > > (gdb) handle SIGTRAP > SIGTRAP is used by the debugger. > Are you sure you want to change it? (y or n) > > Now answer "yes" on UI #1. What happens? > > GDB incorrectly changes SIGTRAP, not SIGINT, and > probably gets the UIs messed up. Indeed, it crashes too! (gdb) handle SIGINT SIGINT is used by the debugger. Are you sure you want to change it? (y or n) y y readline: readline_callback_read_char() called with no handler! [1] 15931 abort (core dumped) ./gdb -nx test -ex "new-ui console /dev/pts/27" -ex "start" -ex "b main" > Why? > > Because we'd have this in pseudo-backtrace: > > #0 gdb_readline_wrapper > #1 defaulted_query // for UI #2 > #2 handle_command > #3 execute_command ("handle SIGTRAP" .... > #4 stdin_event_handler // input on UI #2 > #5 gdb_do_one_event > #7 gdb_readline_wrapper > #8 defaulted_query // for UI #1 > #9 handle_command > #10 execute_command ("handle SIGINT" .... > #11 stdin_event_handler // input on UI #1 > #12 gdb_do_one_event > #13 gdb_readline_wrapper > > So answering "yes", returns the read input to the > query in frame #1... > > So that hunk addresses this by only allowing queries on > the main UI, similarly to how we only enable readline > on the main UI. The idea of only allowing queries on the main UI was also brought up in discussions here. I initially thought it was a bit too arbitrary, and that we would want some on secondary UIs. However, realistically, the only use case of new-ui we know about so far is CLI as the main UI and MI as a secondary UI. Plus, since there won't be readline on secondary CLI UIs, they will already be "dumbed" down, so I think it's fine to not have queries as well... So I think your suggestion is good, at least until we discover that people use multiple CLI UIs at the same time. Note that we can trigger the same bug you mentioned higher with pagination. For example, I have a huge list of breakpoints and I do "info break" on UI #1 then on UI #2. Pressing enter on UI #1 unblocks UI #2, and the following enter crashes GDB. So should we also say that pagination is only allowed on the main UI for the same reasons? > So I think that to support multiple queries like that > the simplest / most natural would be to make each > UI above run on its own thread, so that each would have > its own independent stack/frames. Indeed. That represents a tremendous amount of work I imagine, putting the proper locking mechanisms in place... And if you are holding a lock while the query is issued, it would still block some other things.
On 17-02-10 01:36 PM, Pedro Alves wrote: > On 02/10/2017 06:07 PM, Pedro Alves wrote: >> On 02/10/2017 05:44 PM, Pedro Alves wrote: >> >>> OK, I found the branch. Pushed here now: >>> >>> https://github.com/palves/gdb/commits/palves/console-pagination >> >> And re-reading the branch again, I noticed this hunk: >> >> @@ -1255,7 +1258,9 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) >> way, important error messages don't get lost when talking to GDB >> over a pipe. */ >> if (current_ui->instream != current_ui->stdin_stream >> - || !input_interactive_p (current_ui)) >> + || !input_interactive_p (current_ui) >> + /* We can't handle nested queries. */ >> + || current_ui != main_ui) >> { >> old_chain = make_cleanup_restore_target_terminal (); >> >> @@ -2045,36 +2050,48 @@ begin_line (void) >> } >> } >> >> This is similar to your patch, but it handles something your >> version doesn't, I think. That is the case of handling the secondary >> channel being a CLI interpreter, not an MI one, and that UI >> having been started on a terminal. > > So with with all the rationale I've thrown out, and idea > that we shouldn't see a query anyway, and considering that > real MI commands shouldn't really query anyway (nobody sees > the query output), it may be actually impossible to find > a way to this with MI on the secondary channel, that will > remain stable going forward. Hmm. I'm not sure I understand this part: > it may be actually impossible to find > a way to this with MI on the secondary channel, that will > remain stable going forward. > So how about using the hunk shown above, which should handle > your case as well, and while at it write a test that > uses CLI in the secondary UI instead, making sure a query > is auto-answered in that UI? > > Thanks, > Pedro Alves > That's fine with me.
On 02/10/2017 07:02 PM, Simon Marchi wrote: > On 17-02-10 12:44 PM, Pedro Alves wrote: >> And it is at this point that I thought that it is odd to >> query and ask for user input deep down inside the record layer, >> while handling some asynchronous execution event -- i.e., >> deep down inside handle_inferior_event. If we're buffering >> output, when the user won't see the query anyway. >> Hmm, now that I think of it, the "Do you want to auto delete >> previous execution" query wouldn't be converted to an error, >> but instead to a something like TARGET_WAITKIND_NO_HISTORY, >> I suppose. > > And TARGET_WAITKIND_NO_HISTORY would cause a user-visible stop? Yes. TARGET_WAITKIND_NO_HISTORY already exists. It causes a user-visible stop with "No more reverse-execution history". This case is similar, except it's more like "No more SPACE in reverse-execution history". > (I'm processing the rest and will reply to your latest message) Thanks, Pedro Alves
On 02/10/2017 07:05 PM, Simon Marchi wrote: > On 17-02-10 01:07 PM, Pedro Alves wrote: >> So I think that to support multiple queries like that >> the simplest / most natural would be to make each >> UI above run on its own thread, so that each would have >> its own independent stack/frames. > > Indeed. That represents a tremendous amount of work I imagine, > putting the proper locking mechanisms in place... And if you > are holding a lock while the query is issued, it would still > block some other things. I had it working with a GIL/BKL-style lock, in the original "new-console" prototype that was later rewritten into what is "new-ui" today. I.e,. even though we'd have multiple threads, only one thread really runs at a time. The idea was that we'd start with a big lock, and then over time break down the lock into more finer-grained locks. Here: https://github.com/palves/gdb/commits/palves/console-extra - A thread per UI. See the "Start a thread for each UI" patch. - Per-UI readline (with a giant readline hack) - Per-UI nurses/TUI instance (it really works!) :-) The trouble is that this then trips on another nasty problem: All ptrace calls targeting a process must be issued from the thread that first attached to that inferior process. The kernel rejects the ptrace call otherwise eith EIO/EINVAL or some such, I don't recall which. So on that branch, with native debugging, if you start the inferior on UI #1, and then try to read memory from UI #2, it fails... If you instead try the same against gdbserver, it all works, because in that case gdbserver handles the ptrace calls, not gdb, so all ptrace calls come from the same thread in that case. So for native debugging, we'd need to marshall all ptrace requests through the same thread... Thanks, Pedro Alves
On 02/10/2017 07:07 PM, Simon Marchi wrote: > I'm not sure I understand this part: > >> > it may be actually impossible to find >> > a way to this with MI on the secondary channel, that will >> > remain stable going forward. If we write a test today that relies on the output of a query that arguably shouldn't exist, then when we finally remove the query call, we must remove the test as well. Thanks, Pedro Alves
On 17-02-10 02:19 PM, Pedro Alves wrote: > On 02/10/2017 07:05 PM, Simon Marchi wrote: >> On 17-02-10 01:07 PM, Pedro Alves wrote: > >>> So I think that to support multiple queries like that >>> the simplest / most natural would be to make each >>> UI above run on its own thread, so that each would have >>> its own independent stack/frames. >> >> Indeed. That represents a tremendous amount of work I imagine, >> putting the proper locking mechanisms in place... And if you >> are holding a lock while the query is issued, it would still >> block some other things. > > I had it working with a GIL/BKL-style lock, in the original > "new-console" prototype that was later rewritten into what > is "new-ui" today. I.e,. even though we'd have multiple > threads, only one thread really runs at a time. The idea > was that we'd start with a big lock, and then over time break > down the lock into more finer-grained locks. > > Here: > > https://github.com/palves/gdb/commits/palves/console-extra > > - A thread per UI. See the "Start a thread for each UI" patch. > - Per-UI readline (with a giant readline hack) > - Per-UI nurses/TUI instance (it really works!) :-) And which thread handles inferior events? > The trouble is that this then trips on another nasty problem: > > All ptrace calls targeting a process must be issued > from the thread that first attached to that inferior process. > The kernel rejects the ptrace call otherwise eith EIO/EINVAL > or some such, I don't recall which. So on that branch, with > native debugging, if you start the inferior on UI #1, and then > try to read memory from UI #2, it fails... If you instead > try the same against gdbserver, it all works, because in that > case gdbserver handles the ptrace calls, not gdb, so all > ptrace calls come from the same thread in that case. > So for native debugging, we'd need to marshall all ptrace > requests through the same thread... A PtraceService... it starts to look just like CDT! :)
On 02/10/2017 07:05 PM, Simon Marchi wrote: > > Note that we can trigger the same bug you mentioned higher with pagination. For > example, I have a huge list of breakpoints and I do "info break" on UI #1 then on > UI #2. I assume you mean that on UI #2 you do the same. > Pressing enter on UI #1 unblocks UI #2, and the following enter crashes > GDB. So should we also say that pagination is only allowed on the main UI for the > same reasons? Yes, I think so. The branch disabled it too, see filtering_enabled. Thanks, Pedro Alves
On 02/10/2017 07:26 PM, Simon Marchi wrote:
> And which thread handles inferior events?
The main thread, IIRC. If you only have one UI,
then everything runs on the main thread like it runs today.
If you have one extra UI, that UI gets its own thread, and
on and on. Baby steps. :-)
Thanks,
Pedro Alves
diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c index 8712c75f39..709bfbbce3 100644 --- a/gdb/cli/cli-interp.c +++ b/gdb/cli/cli-interp.c @@ -356,6 +356,12 @@ cli_interp_base::supports_command_editing () return true; } +bool +cli_interp_base::supports_queries () +{ + return true; +} + static struct gdb_exception safe_execute_command (struct ui_out *command_uiout, char *command, int from_tty) { diff --git a/gdb/cli/cli-interp.h b/gdb/cli/cli-interp.h index de9da83347..f1e966fe77 100644 --- a/gdb/cli/cli-interp.h +++ b/gdb/cli/cli-interp.h @@ -31,6 +31,7 @@ public: void set_logging (ui_file_up logfile, bool logging_redirect) override; void pre_command_loop () override; bool supports_command_editing () override; + bool supports_queries () override; }; /* Make the output ui_file to use when logging is enabled. diff --git a/gdb/interps.h b/gdb/interps.h index 1b9580c118..d328163027 100644 --- a/gdb/interps.h +++ b/gdb/interps.h @@ -76,6 +76,10 @@ public: virtual bool supports_command_editing () { return false; } + /* Returns true if this interpreter supports queries. */ + virtual bool supports_queries () + { return false; } + /* This is the name in "-i=" and "set interpreter". */ const char *name; diff --git a/gdb/utils.c b/gdb/utils.c index 3dc2f03d61..ba1db8a45b 100644 --- a/gdb/utils.c +++ b/gdb/utils.c @@ -1170,12 +1170,15 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args) if (!confirm || server_command) return def_value; + struct interp *interp = current_interpreter (); + /* If input isn't coming from the user directly, just say what question we're asking, and then answer the default automatically. This way, important error messages don't get lost when talking to GDB over a pipe. */ if (current_ui->instream != current_ui->stdin_stream - || !input_interactive_p (current_ui)) + || !input_interactive_p (current_ui) + || !interp->supports_queries ()) { old_chain = make_cleanup_restore_target_terminal ();