From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Tim Ruffing Newsgroups: gmane.emacs.bugs Subject: bug#68272: [PATCH] Fix -1 leaking from C to lisp in 'read-event' etc. Date: Fri, 02 Feb 2024 19:04:45 +0100 Message-ID: References: <46480759b6d89b5a4864e8ee1b986817366a56e5.camel@timruffing.de> <83wmsmucuo.fsf@gnu.org> <1b3fa12138838a3fe5643a9e76a65d32a677e34d.camel@timruffing.de> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="8329"; mail-complaints-to="usenet@ciao.gmane.io" Cc: Eli Zaretskii , 68272@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Feb 02 19:06:17 2024 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1rVxvc-0001xC-Ia for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 02 Feb 2024 19:06:16 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1rVxvF-0001NY-49; Fri, 02 Feb 2024 13:05:53 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1rVxvD-0001MN-Eq for bug-gnu-emacs@gnu.org; Fri, 02 Feb 2024 13:05:51 -0500 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1rVxvD-0004UO-6F for bug-gnu-emacs@gnu.org; Fri, 02 Feb 2024 13:05:51 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1rVxvN-0008KT-Lu for bug-gnu-emacs@gnu.org; Fri, 02 Feb 2024 13:06:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Tim Ruffing Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 02 Feb 2024 18:06:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 68272 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 68272-submit@debbugs.gnu.org id=B68272.170689710831932 (code B ref 68272); Fri, 02 Feb 2024 18:06:01 +0000 Original-Received: (at 68272) by debbugs.gnu.org; 2 Feb 2024 18:05:08 +0000 Original-Received: from localhost ([127.0.0.1]:45540 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rVxuW-0008Iy-40 for submit@debbugs.gnu.org; Fri, 02 Feb 2024 13:05:08 -0500 Original-Received: from mout-p-101.mailbox.org ([2001:67c:2050:0:465::101]:35042) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1rVxuT-0008IM-Ue for 68272@debbugs.gnu.org; Fri, 02 Feb 2024 13:05:07 -0500 Original-Received: from smtp1.mailbox.org (smtp1.mailbox.org [IPv6:2001:67c:2050:b231:465::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mout-p-101.mailbox.org (Postfix) with ESMTPS id 4TRNvz4LP0z9st0; Fri, 2 Feb 2024 19:04:47 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=timruffing.de; s=MBO0001; t=1706897087; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:autocrypt:autocrypt; bh=z1TUrxRwgmMOqEOz1PH1X8KWWGjOMtxbcKUKx5SoAN0=; b=rV8dHVmYeeUKxpe/FRVYJj6Utcup/MUXfSpzpaKK6qGUsiwnB42GadEKXqzMiXmFyTf1lL n4C1f9kenfgCw3HvfTiHnPnIBrrtJKL2R6nqq7KRPs1N5zXA2aBDujRxlwsdUvCgPuT+ND I4wJB143FCvL+pCkW36r9IYIvT/QAtGlTIrHMlPeoTbLvK8RgyiHL413ClI3LBw+qerPAI pZb1sMjhed9BpOla+p3TO1DPbnzS0qFLo783P01XyePBkVG6soQCLP3VyCtS/OXF6Bm/Th Wbs0kSm6G07xEdkWBldqP9J10hndYyfvSR2bi2ZxCJWrd7dPvAT/FW7NPXNjag== In-Reply-To: Autocrypt: addr=crypto@timruffing.de; prefer-encrypt=mutual; keydata=mQINBFz4LCMBEADLgtVg3uT+kybmXDPpXMvd8KBhTfAL5DP6umC9hkv/WHnfbCOUujhyvBljckcExAFr7tDYSgIjqa0L32SCT0NEaeY/s3WOYIacjBIEjTrgt01401lOWoX3XeYTWOlVUmUg+4iJPmBSPaj2bJR9Sq6NZhQjQ8K24VMtUNMiDeIIcstLkvQ4ZWkSuBUQJrJ0gUCZcUHNEyyGyZj1HOVGqGK7hTIiT1TfAgYKDDzk955LzgxbmATJWQLD7AGUIjKf/s418PTxI7Hh5ptH+Rq30+wkfvzJumYgkWUzeV6jzlOST5LkrFWQTfCXNvFNxSI9FVKjDIJZ7nQlgd+qNpGop90S3UqA8ofoG9liJm/jmbgIfJTgIiJpulycJD90PyJiWxtGshuZnHjCpkmU5vc1ZbuYyzH2wLoABSBsjy3Tb/25W2mnYnsOcVo1sWOGl+08Lb63ocVYGY27OrAIsv35pS/gMSGcJVg/EmPIM4+PmjeOxDlrJEW+8YzjKV9XtDv6VcBT1/OcA64knWC7JAGf0CGRodpolDjyfFRLOPV2/UbyOMJZjkxKTtV0je/RiMTupIHWcmimkvzpNo8D8U+Ac7KTPuPBrbj8EWeTbd/sK6bncjPL2DLomov0gCg/qlgObYmZ834+tQcThIBi3cj1cRj/0yKPy1uHgk2P2jO5i9AXGQARAQABtB9UaW0gUnVmZmluZyA8dGltQHRpbXJ1ZmZpbmcuZGU+iQJXBBMBCABBAhsBBQsJCAcCBhUKCQgLAgQWAgMBAh4 BAheAAhkBFiEECeA/hxCS5A4QbpArM7yGq4D/ X-Rspamd-Queue-Id: 4TRNvz4LP0z9st0 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:279349 Archived-At: On Sat, 2024-01-13 at 12:40 -0500, Stefan Monnier wrote: > I tend to agree that it's closer to the ideal behavior (even though > I still don't know what that is :-). I think that's the crucial question for now. I'm happy to follow up with an updated patch, but we should first see in which direction we want to head. > I think the problem is that we haven't clearly decided what > `execute-kbd-macro` is supposed to do. The problem is that the way > it's > expected to work implies a form of "nesting" such that at the end of > processing those events we return from the function, but sometimes > that's not what happens. E.g. a kmacro can end in the middle of > a recursive edit. >=20 > Your patch brings is closer to a non-nesting semantics, a bit as if > `execute-kbd-macro` pushed all those events into the incoming input > queue and returned immediately. >=20 > Maybe that's what `execute-kbd-macro` should do, really: push all the > events into `unread-command-events` and return. Just to make sure we're on same page: You mean it should (maybe) push the events into `unread-command-events` assuming LIFO semantics?=20 That is, in simple cases when there's no lisp code that reads events explicitly (`read-char` etc) and you have a kmacro A that executes another kmacro B, the it's still the case that we get the normal nesting semantics. In other words, consider the following kmacros: A: read char x, read char y (which does nothing but trigger B), read char z B: read char b Then executing A will be as if the user typed "x b z" explicitly. I think your proposal makes sense. It's simple, it does "what you'd expect" in simple cases, and it seems compatible to the current behavior, up to the -1 bug. (And don't think there's a natural expectation for cases where we hit -1.)=20 > But then, how would we > do the boolean tests for `executing-kbd-macro` which we perform at > various places? [ I guess we could label/wrap the events such that > they > get executed in a special way (which let-binds `executing-kbd-macro` > to > t? ] Yeah, this sounds reasonable (at least at first glance :)).=20 > Seeing how `calc.el` used the -1 to signal an error (i.e. forcing > kmacros to contain "complete sequences"), maybe a half-way > behavior between the current one and your new one would be for > `read_char` to return -1 (or rather a more explicit `end-of-kbd- > macro`) > event *once* and then on the next call to go on and read from > the keyboard. Hm, indeed. But to be honest, I'm not convinced that lisp code should be able to distinguish this at all when trying to get an event. (If you *really* want to detect this, you could still check `executing-kbd- macro` after reading an event and see if it has changed from non-nil to nil.) Moreover, it would make the C code more complicated, because we'll probably have to add another state variable that captures if we've returned -1 (or a symbol) already or not.=20 Another compromise, which I find nicer, is to introduce a new variant of `read-char` (or add a flag) that suppresses the -1 entirely. We'd probably still need the state variable in the C code, but at least this won't change anything for existing callers. We could even keep the - 1... Well okay, assuming we agree that the non-nesting semantics is in general what we want to have, what should we (or I) do now? 1. Work on an improved version of my patch, which brings it closer to the ideal semantics, but doesn't touch too much of the code? 2. Write a new patch that implements the "full" non-nesting semantics suggest above? 3. Rewrite my patch to do something in the middle? (Returning -1 once, or have a boolean flag that fixes the -1.) 4. Do nothing because all of it is too dangerous. If anything, I think option 3 with a flag beats 4, because 3 won't be too dangerous then. But for the rest, I'm really unsure (and anyway, I don't think I'm the one to make the call). 2 sounds nice, but do you think there will be even more dragons with 2 than with 1? It will be an even larger change for sure... ---- More inline replies: > IIUC, with your patch, we have the following scenario: > - Say we're inside a kmacro with N events left to execute. > - Dbus reads those N events and stashes them them onto `unread- > command-events`. > - Dbus finally can read the actual dbus event and does its thing. > - Good! > - But now `at_end_of_macro_p` will return true, even though we've yet > to > execute the last N events. We'll presumably still execute them > (since they're in `unread-command-events`) but that won't be > considered as coming from a kmacro any more. This matches my understanding. My thinking is that this is not a big deal in this specific case. The dbus code currently relies on the idea of reading events and putting them back to `unread-command-events`. While the patch affects the behavior, it doesn't change anything about the fact that this is an ugly hack anyway. So I'm not really convinced that we should care about this change of behavior too much, as long as we don't break anything.=20 >=20 > > And I'm also aware that it doesn't spark confidence when someone > > with > > (almost) zero code contributions submits a patch of this kind. >=20 > [ Actually, such carefully written code and (even more so) the > =C2=A0 convention-abiding commit messages coming from someone who's not > =C2=A0 a regular contributor spiked my ears.=C2=A0 It made me think > =C2=A0 that this is either a carefully crafted trojan, or it's a really > =C2=A0 welcome new kid on the block :-)=C2=A0 ] >=20 Hehe, thanks for the kind words. I'll submit the trojan in the next patch then. ;) On a more serious note, I took the time to dig deeply here, but I don't think I'll have the bandwidth regularly (as you see from my response time...)=20 By the way, the CONTRIBUTE file is pretty long, but it's also pretty good and clear.=20 > Yes, I think we should document in a comment somewhere how the end of > kmacro is handled from a "global" perspective: what `read_char` > does when it's the case, where/when it's supposed to be detected and > how > this "signal" is propagated from there to the corresponding call to > `execute-kbd-macro`, how that relates to setting `executing-kbd- > macro` > back to nil, ... >=20 Agreed. > "-1 is not propagated to lisp" doesn't say what happens in its stead. > What this does really is transparently continue reading input from > the > keyboard when reaching the end of a kmacro. >=20 Agreed, I can change it. But let's first decide what we would like to do in general. Best, Tim