From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Alan Mackenzie Newsgroups: gmane.emacs.devel Subject: Re: [Emacs-diffs] master 5b5f441: read_key_sequence: correct the handling of raw_keybuf in recursive calls Date: Thu, 23 Nov 2017 18:10:17 +0000 Message-ID: <20171123181017.GA5167@ACM> References: <20171120181210.7946F20416@vcs0.savannah.gnu.org> <20171120195918.GB3917@ACM> <20171120203004.GC3917@ACM> <20171122192522.GA4288@ACM> <20171122210426.GB4288@ACM> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: blaine.gmane.org 1511460870 7810 195.159.176.226 (23 Nov 2017 18:14:30 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Thu, 23 Nov 2017 18:14:30 +0000 (UTC) User-Agent: Mutt/1.7.2 (2016-11-26) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Nov 23 19:14:25 2017 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1eHw0y-0001Ex-55 for ged-emacs-devel@m.gmane.org; Thu, 23 Nov 2017 19:14:20 +0100 Original-Received: from localhost ([::1]:45584 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eHw12-0005a7-B4 for ged-emacs-devel@m.gmane.org; Thu, 23 Nov 2017 13:14:24 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:44702) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eHw0r-0005Zm-S1 for emacs-devel@gnu.org; Thu, 23 Nov 2017 13:14:15 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eHw0o-000063-Tb for emacs-devel@gnu.org; Thu, 23 Nov 2017 13:14:13 -0500 Original-Received: from colin.muc.de ([193.149.48.1]:27794 helo=mail.muc.de) by eggs.gnu.org with smtp (Exim 4.71) (envelope-from ) id 1eHw0o-00005E-Ht for emacs-devel@gnu.org; Thu, 23 Nov 2017 13:14:10 -0500 Original-Received: (qmail 12798 invoked by uid 3782); 23 Nov 2017 18:14:09 -0000 Original-Received: from acm.muc.de (p548C703D.dip0.t-ipconnect.de [84.140.112.61]) by colin.muc.de (tmda-ofmipd) with ESMTP; Thu, 23 Nov 2017 19:14:08 +0100 Original-Received: (qmail 5243 invoked by uid 1000); 23 Nov 2017 18:10:17 -0000 Content-Disposition: inline In-Reply-To: X-Delivery-Agent: TMDA/1.1.12 (Macallan) X-Primary-Address: acm@muc.de X-detected-operating-system: by eggs.gnu.org: FreeBSD 9.x [fuzzy] X-Received-From: 193.149.48.1 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:220406 Archived-At: Hello, Stefan. On Thu, Nov 23, 2017 at 09:35:03 -0500, Stefan Monnier wrote: [ .... ] > > How about this idea? Split Fx_popup_menu into two pieces, Fx_popup_menu > > which just initialises the global raw key buffer then calls > > sub_x_popup_menu. read_char_x_menu_prompt (see above list) would then > > call sub_x_popup_menu to avoid emptying the global buffer. > > Other than that, the global raw key buffer would be initialised in > > command_loop_1 and (possibly) read_key_sequence_vs. > > This way, everything which appends to the raw key buffer could just use > > the global variables, without any dangerous shenanigans on the stack. > Hmm... that might work, but I still can't imagine what comment I could > write next to the code to explain/prove that it's safe. It's as safe as the original, since the only buffer space used is the original raw_keybuf, indexed by raw_keybuf_count. The only change is to initialise raw_keybuf_count in different places, to allow the recursive call to read_key_sequence not to overwrite the current buffer contents. > I think a record_unwind_protect is the easiest way to make it safer. Please try the patch below (which still needs some commenting added). > Yet, with the new concurrency feature, the unwind might reset the > global var to a pointer into a stack area on another thread, which may > have gotten stale in the mean time. If we've got two tasks simultaneously accessing that global buffer, we're in deep trouble anyway. Obviously some sort of lock would need to be applied to this and other global things. > Stefan The following patch (which applies to master without 5b5f441) implements the above idea: diff --git a/src/keyboard.c b/src/keyboard.c index 57757cf211..c32e8feb97 100644 --- a/src/keyboard.c +++ b/src/keyboard.c @@ -1365,6 +1365,7 @@ command_loop_1 (void) Vthis_command_keys_shift_translated = Qnil; /* Read next key sequence; i gets its length. */ + raw_keybuf_count = 0; i = read_key_sequence (keybuf, ARRAYELTS (keybuf), Qnil, 0, 1, 1, 0); @@ -8860,6 +8861,11 @@ test_undefined (Lisp_Object binding) && EQ (Fcommand_remapping (binding, Qnil, Qnil), Qundefined))); } +void init_raw_keybuf_count (void) +{ + raw_keybuf_count = 0; +} + /* Read a sequence of keys that ends with a non prefix character, storing it in KEYBUF, a buffer of size BUFSIZE. Prompt with PROMPT. @@ -8971,7 +8977,7 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, Lisp_Object prompt, /* List of events for which a fake prefix key has been generated. */ Lisp_Object fake_prefixed_keys = Qnil; - raw_keybuf_count = 0; + /* raw_keybuf_count = 0; */ last_nonmenu_event = Qnil; @@ -9837,6 +9843,7 @@ read_key_sequence_vs (Lisp_Object prompt, Lisp_Object continue_echo, cancel_hourglass (); #endif + raw_keybuf_count = 0; i = read_key_sequence (keybuf, ARRAYELTS (keybuf), prompt, ! NILP (dont_downcase_last), ! NILP (can_return_switch_frame), 0, 0); diff --git a/src/keyboard.h b/src/keyboard.h index 662d8e4a4f..c232e778e2 100644 --- a/src/keyboard.h +++ b/src/keyboard.h @@ -438,6 +438,7 @@ extern unsigned int timers_run; extern bool menu_separator_name_p (const char *); extern bool parse_menu_item (Lisp_Object, int); +extern void init_raw_keybuf_count (void); extern KBOARD *allocate_kboard (Lisp_Object); extern void delete_kboard (KBOARD *); extern void not_single_kboard_state (KBOARD *); diff --git a/src/menu.c b/src/menu.c index d569b4b29b..2715f9ce82 100644 --- a/src/menu.c +++ b/src/menu.c @@ -1475,30 +1475,9 @@ emulate_dialog_with_menu (struct frame *f, Lisp_Object contents) return Fx_popup_menu (newpos, list2 (prompt, contents)); } -DEFUN ("x-popup-dialog", Fx_popup_dialog, Sx_popup_dialog, 2, 3, 0, - doc: /* Pop up a dialog box and return user's selection. -POSITION specifies which frame to use. -This is normally a mouse button event or a window or frame. -If POSITION is t, it means to use the frame the mouse is on. -The dialog box appears in the middle of the specified frame. - -CONTENTS specifies the alternatives to display in the dialog box. -It is a list of the form (DIALOG ITEM1 ITEM2...). -Each ITEM is a cons cell (STRING . VALUE). -The return value is VALUE from the chosen item. - -An ITEM may also be just a string--that makes a nonselectable item. -An ITEM may also be nil--that means to put all preceding items -on the left of the dialog box and all following items on the right. -\(By default, approximately half appear on each side.) - -If HEADER is non-nil, the frame title for the box is "Information", -otherwise it is "Question". - -If the user gets rid of the dialog box without making a valid choice, -for instance using the window manager, then this produces a quit and -`x-popup-dialog' does not return. */) - (Lisp_Object position, Lisp_Object contents, Lisp_Object header) +static Lisp_Object +x_popup_dialog_1 (Lisp_Object position, Lisp_Object contents, + Lisp_Object header) { struct frame *f = NULL; Lisp_Object window; @@ -1571,6 +1550,35 @@ for instance using the window manager, then this produces a quit and return emulate_dialog_with_menu (f, contents); } +DEFUN ("x-popup-dialog", Fx_popup_dialog, Sx_popup_dialog, 2, 3, 0, + doc: /* Pop up a dialog box and return user's selection. +POSITION specifies which frame to use. +This is normally a mouse button event or a window or frame. +If POSITION is t, it means to use the frame the mouse is on. +The dialog box appears in the middle of the specified frame. + +CONTENTS specifies the alternatives to display in the dialog box. +It is a list of the form (DIALOG ITEM1 ITEM2...). +Each ITEM is a cons cell (STRING . VALUE). +The return value is VALUE from the chosen item. + +An ITEM may also be just a string--that makes a nonselectable item. +An ITEM may also be nil--that means to put all preceding items +on the left of the dialog box and all following items on the right. +\(By default, approximately half appear on each side.) + +If HEADER is non-nil, the frame title for the box is "Information", +otherwise it is "Question". + +If the user gets rid of the dialog box without making a valid choice, +for instance using the window manager, then this produces a quit and +`x-popup-dialog' does not return. */) + (Lisp_Object position, Lisp_Object contents, Lisp_Object header) +{ + init_raw_keybuf_count (); + return x_popup_dialog_1 (position, contents, header); +} + void syms_of_menu (void) { -- Alan Mackenzie (Nuremberg, Germany).