From debbugs-submit-bounces@debbugs.gnu.org Sat Jan 18 07:19:03 2025 Received: (at submit) by debbugs.gnu.org; 18 Jan 2025 12:19:03 +0000 Received: from localhost ([127.0.0.1]:40625 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1tZ7n5-00020C-4z for submit@debbugs.gnu.org; Sat, 18 Jan 2025 07:19:03 -0500 Received: from lists.gnu.org ([2001:470:142::17]:57208) by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1tZ7n2-0001zN-ML for submit@debbugs.gnu.org; Sat, 18 Jan 2025 07:19:01 -0500 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 1tZ7mk-0005OG-06 for bug-gnu-emacs@gnu.org; Sat, 18 Jan 2025 07:18:42 -0500 Received: from mail-4322.protonmail.ch ([185.70.43.22]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1tZ7mg-0000hf-D7 for bug-gnu-emacs@gnu.org; Sat, 18 Jan 2025 07:18:41 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1737202713; x=1737461913; bh=JvONP/pzYcn4gEmr9sxQp8jr3CKu/OCMNo2/xFxQoVA=; h=Date:To:From:Subject:Message-ID:Feedback-ID:From:To:Cc:Date: Subject:Reply-To:Feedback-ID:Message-ID:BIMI-Selector: List-Unsubscribe:List-Unsubscribe-Post; b=BlQJmnjj5nBc3FNEuth7Vy4En1nNm2blFRKf1kfP2yMTX+xg3YlUgdcO0r1p7kgPO o7mByBj+BNB6GUG9tC0YoPnv7Hvg83KwQn3ctWcMRUoS4/Yzm71O+dzcu5KEzbO8yG 4BhuUalBv68g/UGBNmCtkkeScQbfZ05xj9iFwf62RwcH8hcY1YceTTCo49MmyWyqOg ZbfWdLAWWjlWbQzOG9v7T7/zzz3hre9yL+z3c0bOoWw5N1TYGS0OeFUwJFoLQgRa8+ SHiIX+hYpqKWj1fX3IlQbp/KpZOq3I5swiZw+xM73tYprQdp/YPm7rg3rJ1ye3YVK8 FOGiYrZJJ7Bbg== Date: Sat, 18 Jan 2025 12:18:27 +0000 To: bug-gnu-emacs@gnu.org From: Pip Cet Subject: Minor safety improvements to fns.c/eval.c Message-ID: <87jzasco3g.fsf@protonmail.com> Feedback-ID: 112775352:user:proton X-Pm-Message-ID: 50a085b07ef6c28b76ad16fa80027933a50a4642 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Received-SPF: pass client-ip=185.70.43.22; envelope-from=pipcet@protonmail.com; helo=mail-4322.protonmail.ch X-Spam_score_int: -38 X-Spam_score: -3.9 X-Spam_bar: --- X-Spam_report: (-3.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-1.787, RCVD_IN_VALIDITY_CERTIFIED_BLOCKED=0.001, RCVD_IN_VALIDITY_RPBL_BLOCKED=0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-Spam-Score: 1.0 (+) X-Debbugs-Envelope-To: submit X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -0.0 (/) This is a spin-off from bug#75584, originally reported in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=3D75584#27 fns.c contains some minor bugs which can cause crashes for buggy Elisp. While it's conceivable that one of them could happen by accident, it doesn't affect the pdumper build, so it is still very unlikely. They are: 1. plist-get and plist-put assume that the cdr of a cons cell is also a cons cell. They check this, then call out to Lisp, then rely on the fact. However, the Lisp code can call setcdr and turn the cdr into a non-cons cell, which causes a crash. 2. Fntake, Fsort, and Fwidget_put modify user-supplied cons cells with XSETCAR/XSETCDR. They do not check that the cons cell isn't "pure", in which case writing to it may cause crashes. In the case of Fsort, this is likely to happen when a user accidentally attempts to sort a (partially) pure list in-place. 3. Fsetq does the same thing to the lexical environment, which may include a pure cons cell establishing a binding. As (2) and (3) will become non-bugs once purespace is removed, I would like to propose not fixing them for now. (1), however, needs a fix. Also, just as importantly, it needs tests. Unfortunately, ert is not really set up very well for tests that may crash. My proposal will be to: 1. Give such tests a :crash tag 2. Introduce a should-not-crash macro which succeeds if the form it evaluates returns in any way, whether by error or not. 3. Modify ert.el to print when a :crash test is about to start running. This allows us to identify the crashing test. 4. Deviate from the current logical test order and put crash tests last. It's conceivable that if a once-fixed crash reoccurs, the reason is a simple bug that may show up in regular tests, too. If two tests fail and one of them crashes the test run, it's better for the first failure to have been reported first. I'll send a patch once this has a bug number. From debbugs-submit-bounces@debbugs.gnu.org Sat Jan 18 09:20:59 2025 Received: (at submit) by debbugs.gnu.org; 18 Jan 2025 14:20:59 +0000 Received: from localhost ([127.0.0.1]:40783 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1tZ9h4-00026u-Qp for submit@debbugs.gnu.org; Sat, 18 Jan 2025 09:20:59 -0500 Received: from lists.gnu.org ([2001:470:142::17]:35966) by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1tZ9gz-000266-DG for submit@debbugs.gnu.org; Sat, 18 Jan 2025 09:20:56 -0500 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 1tZ9gu-0003k9-2l for bug-gnu-emacs@gnu.org; Sat, 18 Jan 2025 09:20:48 -0500 Received: from mail-10629.protonmail.ch ([79.135.106.29]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1tZ9gr-000745-U1 for bug-gnu-emacs@gnu.org; Sat, 18 Jan 2025 09:20:47 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1737210043; x=1737469243; bh=szUo5+mKuNJniVDXzaiRHpaMlNJspiYpFU77DsuNlT0=; h=Date:To:From:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post; b=IWTI48pETWjoXxAJk/Z5DgUUMkiJLJjiWhD1elo+It1mZiIY42woFL30dJ8Ri+75r 6vx0GLqL45J1g/lPVW+8okPcrm9/PlMp5shPzxGkO5rsMgje/nuFJtYYk1yLwXt5DL 9gsa/gScKZY/CRVJOzJhtJg8ntCsUOI0/FA2R9vphXd9DMr8wvAGKa4TgsnRVQpWLE gUu9QMia4izSgCTUK2l13qEfIbNMqZ8Cp/gGP6zvcVSyALlyq44cIaK8wcm3FSLjwH J5E/K4Eujj2wcZqdaXgRPQsdigwlEuwEOcZew2rWYPSqJp716AntGaebHZOBbP+U2q NORvCz1nlMebg== Date: Sat, 18 Jan 2025 14:20:39 +0000 To: bug-gnu-emacs@gnu.org, 75648@debbugs.gnu.org From: Pip Cet Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c Message-ID: <87cygkcifu.fsf@protonmail.com> In-Reply-To: <87jzasco3g.fsf@protonmail.com> References: <87jzasco3g.fsf@protonmail.com> Feedback-ID: 112775352:user:proton X-Pm-Message-ID: 291ee56f1b2fecc2670bcb32d5c87f21a24c71ac MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Received-SPF: pass client-ip=79.135.106.29; envelope-from=pipcet@protonmail.com; helo=mail-10629.protonmail.ch X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H5=0.001, RCVD_IN_MSPIKE_WL=0.001, RCVD_IN_VALIDITY_CERTIFIED_BLOCKED=0.001, RCVD_IN_VALIDITY_RPBL_BLOCKED=0.001, SPF_HELO_PASS=-0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-Spam-Score: 1.0 (+) X-Debbugs-Envelope-To: submit X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -0.0 (/) "Pip Cet via \"Bug reports for GNU Emacs, the Swiss army knife of text edit= ors\"" writes: > 1. Give such tests a :crash tag > 2. Introduce a should-not-crash macro which succeeds if the form it > evaluates returns in any way, whether by error or not. > 3. Modify ert.el to print when a :crash test is about to start running. > This allows us to identify the crashing test. > 4. Deviate from the current logical test order and put crash tests last. > It's conceivable that if a once-fixed crash reoccurs, the reason is a > simple bug that may show up in regular tests, too. If two tests fail > and one of them crashes the test run, it's better for the first failure > to have been reported first. > > I'll send a patch once this has a bug number. Patch 1 adds :crash tests to ert.el. Patch 2 adds new tests which crash if the bug is present. Patch 3 fixes the actual bug. Remarks: * should-not-crash needs to be written carefully to avoid byte compiler warnings and, in the general case, ignore errors. * printing the test name before the crash makes a lot of sense to me, and it's easier to do in ert-run-tests-batch rather than in should-not-crash itself. * plist_put wasn't buggy, but I applied analogous changes to keep it synchronized with Fplist_put. commit a4bf048d1f36680d37ffbd4c54c67ea13840117c Author: Pip Cet Date: Sat Jan 18 12:26:36 2025 +0000 Handle tests known to cause crashes =20 * lisp/emacs-lisp/ert.el (should-not-crash): New macro. (ert-run-tests-batch): When running a test with a :crash tag, print a message first. diff --git a/lisp/emacs-lisp/ert.el b/lisp/emacs-lisp/ert.el index f25ba8a529c..cd25fe6a108 100644 --- a/lisp/emacs-lisp/ert.el +++ b/lisp/emacs-lisp/ert.el @@ -457,6 +457,14 @@ should-error (list :fail-reason "did not signal an error"))))))))) =20 +(cl-defmacro should-not-crash (form) + "Evaluate FORM and ignore its value. + +If Emacs doesn't crash, count it as a success. + +Returns nil." + `(should-not (and (ignore-errors ,form) nil))) + (cl-defmacro ert--skip-when (form) "Evaluate FORM. If it returns t, skip the current test. Errors during evaluation are caught and handled like t." @@ -1439,7 +1447,20 @@ ert-run-tests-batch (message "%s" "")) (when (getenv "EMACS_TEST_JUNIT_REPORT") (ert-write-junit-test-report stats))))) - (test-started) + (test-started + ;; for tests which might crash, the last line of output should + ;; indicate which test we attempted to run. + (cl-destructuring-bind (stats test ) event-args + (and (not ert-quiet) + (memq :crash (ert-test-tags test)) + (let* ((max (prin1-to-string (length (ert--stats-tests stat= s)))) + (format-string (concat "%9s %" + (prin1-to-string (length max)= ) + "s/" max " %S"))) + (message format-string + "started" + (1+ (ert--stats-test-pos stats test)) + (ert-test-name test)))))) (test-ended (cl-destructuring-bind (stats test result) event-args (unless (ert-test-result-expected-p test result) commit f891757a7b71980fc81afff0e818911ad6e1f344 Author: Pip Cet Date: Sat Jan 18 12:27:58 2025 +0000 Add :crash tests for bug#75648 =20 * test/src/fns-tests.el (test-self-modifying-plist-get): (test-self-modifying-plist-put): New tests. diff --git a/test/src/fns-tests.el b/test/src/fns-tests.el index eebc92bfd41..edb919badcd 100644 --- a/test/src/fns-tests.el +++ b/test/src/fns-tests.el @@ -1817,4 +1817,16 @@ fns-value<-bool-vector ;; Undo the flip. (aset b i val))))))))))) =20 +(ert-deftest test-self-modifying-plist-get () + :tags '(:crash) + (let ((l (list :key 'value))) + (should-not-crash (plist-get l :otherkey + #'(lambda (&rest _) (setcdr l nil)))))) + +(ert-deftest test-self-modifying-plist-put () + :tags '(:crash) + (let ((l (list :key 'value))) + (should-not-crash (plist-put l :otherkey 'dummy + #'(lambda (&rest _) (setcdr l nil)))))) + ;;; fns-tests.el ends here commit d70c8d145a79f91b4cdd5f050a7b0f6cca425596 Author: Pip Cet Date: Sat Jan 18 13:41:31 2025 +0000 Fix unlikely crashes for self-modifying plists (bug#75684) =20 * src/fns.c (Fplist_get): Verify 'tail' is always a cons cell. (Fplist_put): Verify 'tail' is always a cons cell. Use 'prev_cdr' to refer to the cons cell which has the value to be modified as its car. (plist_put): Analogous changes, to improve readability. diff --git a/src/fns.c b/src/fns.c index 07df2a5e90e..c07891771bb 100644 --- a/src/fns.c +++ b/src/fns.c @@ -2602,11 +2602,12 @@ DEFUN ("plist-get", Fplist_get, Splist_get, 2, 3, 0= , Lisp_Object tail =3D plist; FOR_EACH_TAIL_SAFE (tail) { - if (! CONSP (XCDR (tail))) + Lisp_Object tail_cdr =3D XCDR (tail); + if (! CONSP (tail_cdr)) =09break; if (!NILP (call2 (predicate, XCAR (tail), prop))) -=09return XCAR (XCDR (tail)); - tail =3D XCDR (tail); +=09return XCAR (tail_cdr); + tail =3D tail_cdr; } =20 return Qnil; @@ -2657,27 +2658,27 @@ DEFUN ("plist-put", Fplist_put, Splist_put, 3, 4, 0= , { if (NILP (predicate)) return plist_put (plist, prop, val); - Lisp_Object prev =3D Qnil, tail =3D plist; + Lisp_Object prev_cdr =3D Qnil; + Lisp_Object tail =3D plist; FOR_EACH_TAIL (tail) { - if (! CONSP (XCDR (tail))) + Lisp_Object tail_cdr =3D XCDR (tail); + if (! CONSP (tail_cdr)) =09break; - if (!NILP (call2 (predicate, XCAR (tail), prop))) =09{ -=09 Fsetcar (XCDR (tail), val); +=09 Fsetcar (tail_cdr, val); =09 return plist; =09} =20 - prev =3D tail; - tail =3D XCDR (tail); + prev_cdr =3D tail =3D tail_cdr; } CHECK_TYPE (NILP (tail), Qplistp, plist); Lisp_Object newcell - =3D Fcons (prop, Fcons (val, NILP (prev) ? plist : XCDR (XCDR (prev)))= ); - if (NILP (prev)) + =3D Fcons (prop, Fcons (val, NILP (prev_cdr) ? plist : XCDR (prev_cdr)= )); + if (NILP (prev_cdr)) return newcell; - Fsetcdr (XCDR (prev), newcell); + Fsetcdr (prev_cdr, newcell); return plist; } =20 @@ -2685,10 +2686,12 @@ DEFUN ("plist-put", Fplist_put, Splist_put, 3, 4, 0= , Lisp_Object plist_put (Lisp_Object plist, Lisp_Object prop, Lisp_Object val) { - Lisp_Object prev =3D Qnil, tail =3D plist; + Lisp_Object prev_cdr =3D Qnil; + Lisp_Object tail =3D plist; FOR_EACH_TAIL (tail) { - if (! CONSP (XCDR (tail))) + Lisp_Object tail_cdr =3D XCDR (tail); + if (! CONSP (tail_cdr)) =09break; =20 if (EQ (XCAR (tail), prop)) @@ -2697,15 +2700,14 @@ plist_put (Lisp_Object plist, Lisp_Object prop, Lis= p_Object val) =09 return plist; =09} =20 - prev =3D tail; - tail =3D XCDR (tail); + prev_cdr =3D tail =3D tail_cdr;; } CHECK_TYPE (NILP (tail), Qplistp, plist); Lisp_Object newcell - =3D Fcons (prop, Fcons (val, NILP (prev) ? plist : XCDR (XCDR (prev)))= ); - if (NILP (prev)) + =3D Fcons (prop, Fcons (val, NILP (prev_cdr) ? plist : XCDR (prev_cdr)= )); + if (NILP (prev_cdr)) return newcell; - Fsetcdr (XCDR (prev), newcell); + Fsetcdr (prev_cdr, newcell); return plist; } =20 From debbugs-submit-bounces@debbugs.gnu.org Sat Jan 18 10:30:13 2025 Received: (at 75648) by debbugs.gnu.org; 18 Jan 2025 15:30:13 +0000 Received: from localhost ([127.0.0.1]:43404 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1tZAm4-0005zE-JD for submit@debbugs.gnu.org; Sat, 18 Jan 2025 10:30:13 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:42906) by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1tZAm1-0005wA-6p for 75648@debbugs.gnu.org; Sat, 18 Jan 2025 10:30:10 -0500 Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1tZAlv-00070S-9g; Sat, 18 Jan 2025 10:30:03 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=Uv3obcqEN22Jhl/1op/rxFgop35kKtlqtmoK9i44MRE=; b=OgbglcBZwGga YWwvqnWThXDL5bnKPfkD2Ta0iXr5tXRH5TxaHdKOxuSadrzepsi2AfKm3ZOA8uWKzVHPLvX2IgWYM 1Yp88e63SjOb18LCpOWa60ruHwGFAXFt28oKaCJkOuK3mWU1kbMBD+oQ3JmcL44M4WQbmrz9OHB8u GXAYO4++471xTJJk07oci/Ciy2NCM8hcX2tfKF4Ia0vOiIZG/toPB0CYog/usmZ9ONcBPSp9WWwbg CvWuOZXXTSVNSAcn+HthLxt36Iz8MlGReK6M7OFlnP4l/AbYGbmQODPvZjd97yHIAD9JtWglpTTXB t/EEaTjUGB9IxIm70qUHkg==; Date: Sat, 18 Jan 2025 17:29:58 +0200 Message-Id: <86cygk9m2x.fsf@gnu.org> From: Eli Zaretskii To: Pip Cet , Michael Albinus , Stefan Monnier , Stefan Kangas , Andrea Corallo In-Reply-To: <87cygkcifu.fsf@protonmail.com> (bug-gnu-emacs@gnu.org) Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c References: <87jzasco3g.fsf@protonmail.com> <87cygkcifu.fsf@protonmail.com> X-Spam-Score: -2.3 (--) X-Debbugs-Envelope-To: 75648 Cc: 75648@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -3.3 (---) > Date: Sat, 18 Jan 2025 14:20:39 +0000 > From: Pip Cet via "Bug reports for GNU Emacs, > the Swiss army knife of text editors" > > "Pip Cet via \"Bug reports for GNU Emacs, the Swiss army knife of text editors\"" writes: > > > 1. Give such tests a :crash tag > > 2. Introduce a should-not-crash macro which succeeds if the form it > > evaluates returns in any way, whether by error or not. > > 3. Modify ert.el to print when a :crash test is about to start running. > > This allows us to identify the crashing test. > > 4. Deviate from the current logical test order and put crash tests last. > > It's conceivable that if a once-fixed crash reoccurs, the reason is a > > simple bug that may show up in regular tests, too. If two tests fail > > and one of them crashes the test run, it's better for the first failure > > to have been reported first. > > > > I'll send a patch once this has a bug number. > > Patch 1 adds :crash tests to ert.el. Hmm... maybe this should be discussed separately, as it's a much broader issue. Can you provide the motivation, both for the new macro (which is basically a one-liner), and for the new tag? I've added some people to the discussion, because of this particular part (maybe we should take this part to a separate thread). > Patch 2 adds new tests which crash if the bug is present. > Patch 3 fixes the actual bug. Thanks. > + (cl-destructuring-bind (stats test ) event-args > + (and (not ert-quiet) > + (memq :crash (ert-test-tags test)) > + (let* ((max (prin1-to-string (length (ert--stats-tests stats)))) > + (format-string (concat "%9s %" > + (prin1-to-string (length max)) > + "s/" max " %S"))) > + (message format-string > + "started" > + (1+ (ert--stats-test-pos stats test)) > + (ert-test-name test)))))) Is it guaranteed that this text will be output before Emacs dies? Maybe we should introduce flush-standard-error, or change the code to print to stdout and use flush-standard-output? > Fix unlikely crashes for self-modifying plists (bug#75684) > > * src/fns.c (Fplist_get): Verify 'tail' is always a cons cell. > (Fplist_put): Verify 'tail' is always a cons cell. Use 'prev_cdr' to > refer to the cons cell which has the value to be modified as its car. > (plist_put): Analogous changes, to improve readability. > > diff --git a/src/fns.c b/src/fns.c > index 07df2a5e90e..c07891771bb 100644 > --- a/src/fns.c > +++ b/src/fns.c > @@ -2602,11 +2602,12 @@ DEFUN ("plist-get", Fplist_get, Splist_get, 2, 3, 0, > Lisp_Object tail = plist; > FOR_EACH_TAIL_SAFE (tail) > { > - if (! CONSP (XCDR (tail))) > + Lisp_Object tail_cdr = XCDR (tail); > + if (! CONSP (tail_cdr)) > break; > if (!NILP (call2 (predicate, XCAR (tail), prop))) > - return XCAR (XCDR (tail)); > - tail = XCDR (tail); > + return XCAR (tail_cdr); > + tail = tail_cdr; > } > > return Qnil; > @@ -2657,27 +2658,27 @@ DEFUN ("plist-put", Fplist_put, Splist_put, 3, 4, 0, > { > if (NILP (predicate)) > return plist_put (plist, prop, val); > - Lisp_Object prev = Qnil, tail = plist; > + Lisp_Object prev_cdr = Qnil; > + Lisp_Object tail = plist; > FOR_EACH_TAIL (tail) > { > - if (! CONSP (XCDR (tail))) > + Lisp_Object tail_cdr = XCDR (tail); > + if (! CONSP (tail_cdr)) > break; > - > if (!NILP (call2 (predicate, XCAR (tail), prop))) > { > - Fsetcar (XCDR (tail), val); > + Fsetcar (tail_cdr, val); > return plist; > } > > - prev = tail; > - tail = XCDR (tail); > + prev_cdr = tail = tail_cdr; > } > CHECK_TYPE (NILP (tail), Qplistp, plist); > Lisp_Object newcell > - = Fcons (prop, Fcons (val, NILP (prev) ? plist : XCDR (XCDR (prev)))); > - if (NILP (prev)) > + = Fcons (prop, Fcons (val, NILP (prev_cdr) ? plist : XCDR (prev_cdr))); > + if (NILP (prev_cdr)) > return newcell; > - Fsetcdr (XCDR (prev), newcell); > + Fsetcdr (prev_cdr, newcell); > return plist; > } > > @@ -2685,10 +2686,12 @@ DEFUN ("plist-put", Fplist_put, Splist_put, 3, 4, 0, > Lisp_Object > plist_put (Lisp_Object plist, Lisp_Object prop, Lisp_Object val) > { > - Lisp_Object prev = Qnil, tail = plist; > + Lisp_Object prev_cdr = Qnil; > + Lisp_Object tail = plist; > FOR_EACH_TAIL (tail) > { > - if (! CONSP (XCDR (tail))) > + Lisp_Object tail_cdr = XCDR (tail); > + if (! CONSP (tail_cdr)) > break; > > if (EQ (XCAR (tail), prop)) > @@ -2697,15 +2700,14 @@ plist_put (Lisp_Object plist, Lisp_Object prop, Lisp_Object val) > return plist; > } > > - prev = tail; > - tail = XCDR (tail); > + prev_cdr = tail = tail_cdr;; > } > CHECK_TYPE (NILP (tail), Qplistp, plist); > Lisp_Object newcell > - = Fcons (prop, Fcons (val, NILP (prev) ? plist : XCDR (XCDR (prev)))); > - if (NILP (prev)) > + = Fcons (prop, Fcons (val, NILP (prev_cdr) ? plist : XCDR (prev_cdr))); > + if (NILP (prev_cdr)) > return newcell; > - Fsetcdr (XCDR (prev), newcell); > + Fsetcdr (prev_cdr, newcell); > return plist; > } This is fine by me, but I wonder whether we should replace the NILP tests with !CONSP, when we are about to take XCAR or XCDR? From debbugs-submit-bounces@debbugs.gnu.org Sat Jan 18 11:25:42 2025 Received: (at 75648) by debbugs.gnu.org; 18 Jan 2025 16:25:42 +0000 Received: from localhost ([127.0.0.1]:43486 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1tZBdl-0008Sl-Ui for submit@debbugs.gnu.org; Sat, 18 Jan 2025 11:25:42 -0500 Received: from mout.gmx.net ([212.227.17.21]:52939) by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1tZBdj-0008SU-4J for 75648@debbugs.gnu.org; Sat, 18 Jan 2025 11:25:40 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmx.de; s=s31663417; t=1737217522; x=1737822322; i=michael.albinus@gmx.de; bh=Pts6OdYlkQU0+fgXoi75ivzYxqfDsdZPlF+yFBj0tW0=; h=X-UI-Sender-Class:From:To:Cc:Subject:In-Reply-To:References:Date: Message-ID:MIME-Version:Content-Type:cc:content-transfer-encoding: content-type:date:from:message-id:mime-version:reply-to:subject: to; b=MEwiGSd40ogkNwXbW4F9aaI1qEr0Bwqvuu+p1hztn6ahTz3RPUxRnUpIjtAjmrH4 uFoXbOcyyLkFhpYwVNZ5PXwE+C2iB1NW6e+YoL/0Q7w0vM4n7/CHTzCbVUejLfxDh 1nMNyHwpqrAVpIUUB028QXJisLDpsX2u6izjygtuzbnWisBi/91jHkHvKXRtCX7HI TdPPkpu1dXk+Izi7P5E4C6N1yehLioSSVDYe+JtAUlh5XjpKlx5OBANhgUWGce/F/ en0e5qs08/izZEj5Att9v+qkLAGd+XNQHAcgsLXl/j3DjFhJY9uOYf85ms0DDBlua X6YejwINO/BEm3P65w== X-UI-Sender-Class: 724b4f7f-cbec-4199-ad4e-598c01a50d3a Received: from gandalf.gmx.de ([185.89.38.155]) by mail.gmx.net (mrgmx105 [212.227.17.168]) with ESMTPSA (Nemesis) id 1MMofc-1troPu0lpk-00HziC; Sat, 18 Jan 2025 17:25:22 +0100 From: Michael Albinus To: Eli Zaretskii Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c In-Reply-To: <86cygk9m2x.fsf@gnu.org> (Eli Zaretskii's message of "Sat, 18 Jan 2025 17:29:58 +0200") References: <87jzasco3g.fsf@protonmail.com> <87cygkcifu.fsf@protonmail.com> <86cygk9m2x.fsf@gnu.org> Date: Sat, 18 Jan 2025 17:25:18 +0100 Message-ID: <8734hgf5sh.fsf@gmx.de> User-Agent: Gnus/5.13 (Gnus v5.13) MIME-Version: 1.0 Content-Type: text/plain X-Provags-ID: V03:K1:sUIkA/WmKOb8xEGfiI654xjzZ2/YwSkVmm4DCLpRaVHzB63iGoM JSfiSEbKIgvzsyntQnJxYTnIx0nLV//PxBDjLWo4p+wXR/8GmBAY9YY4m72AK/+mgZu3/eJ YwGKHi2uh5/rercci/pi21eJRdhmWB/LdZvBxl+QdG7Hg1bWodh/0VNjuvnonka/wnX9yVE 86eN5tTBIaXDPuVdvHTYg== X-Spam-Flag: NO UI-OutboundReport: notjunk:1;M01:P0:Xblbp2XlY5o=;cdHogKuj/CrDEEQf6qkeTeNhYob atzXRdVXttIm9IJ/IBVoDK8VoZGieap79OnFeOOqwZJtfZ7LZD6IBP8THwRRJmcrSpdD+9CmG z5qBPAc+RS4riovSTeqJA37OErp17z11irZfMWARmh4LDTyrwLFttii0hU9qf/G97RoAEEFKt bRTE3miUS2Kx5KxtZEj5fOQNjK5c3Ut5vp/vHnh9NstPpOB1gHTY4lNEI5ybU2ptBX5VW+2lT Aud0lUFRxr51XoVHLoj7988k6NqZjHjYlb6FMMx2TbOuInc53EqJCHldU/bJRo5Z+9uttSohb f6vftqAwiRmvQfQIx5qjB7GWbqqgfmlysELrQj4+NibQnzgqPxW3+l70upy8CQbrnPWpY4f8V VthdWHf7YDe05FSgZn2OYyh/NumteuOaQzJBXKLzTE4imQ5w59zox68bFm/04YKpon/7EXOTA o07N+vrfqiShILUqPmuPEPe9WSThXGZg5+NvMvOaE4+Qs7dxH1/GflcNy93qVuxYXGBADG9+o OL3YAavC3AFYZ6e4dwdJmRhEk74gevk9y/x6WzYcbcnZSd2R8fm1h0iPC6NaPGjHDf2T5o+ln qiQeZRt4+GfW1G1FXBx21QcgX6q4Di1vVGg+jONox2Ufs17AHHqkHKRo5ZsTh6MSFg0Zr43yu 8D4kzKC1jpLqqQv0DEVv1JuPZTUu9CvDE9aIY0sLCOD9hltXBhxLF5SjfKKcVImnHarYZtrfV cw56hj7VKkhjyzimfQ6IvjCPhyBUHGv6wQf5QXgBjiIodNhLyVcL+0iXWKxKFyVcQ/fr/TZbQ eS7ULbJSyei14LCOuzs7ixRYCzZRjzzJeZsI5x8H2pAWX/ESkHNHUn2/hhtYQdGZvmmCgAG6k evSMHTbTQyyp5lLU//lR5ycLVuskUMW8/bcTugCyuZB+rSTotJraeYRW34mPE6G5Fv+4HDNjz KITxJWtqQ/Bq4xIwn0MRy7cr1EPbJBW2WXNxTaIvZUeun7JuQJCfsPkuniQrqmUBRUvkURG7K ep6CT8BwKGgzn+m2zVxQf3rIeV8tIQkuack9Vdvfw4YLDuRHm/gqwntsER7OajZZVVwB7oj2s wIET+TONzRHB5krakaX/jqsu/l1apYmmeDkMICIXndmpfQjDjZWixvlnqmoXc04N3cQWUBuuj 8QzRtzZ5ZWQAxzBrRPt6FdRmlHT+zp+t/dz9aNG/IWw== X-Spam-Score: -0.7 (/) X-Debbugs-Envelope-To: 75648 Cc: Pip Cet , Andrea Corallo , 75648@debbugs.gnu.org, Stefan Monnier , Stefan Kangas X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.7 (-) Eli Zaretskii writes: Hi, >> Patch 1 adds :crash tests to ert.el. > > Hmm... maybe this should be discussed separately, as it's a much > broader issue. Can you provide the motivation, both for the new macro > (which is basically a one-liner), and for the new tag? This patch adds the :crash tag to ert.el. This is unfortune. Until now, there are no predefined tags in ert.el. Do we want to change this? If we need such a property for ert tests, we should use another mechanism. For example, another slot in the defstruct ert-test, like (should-not-crash nil). And your new macro should-not-crash sets this slot to non-nil in its scope. With this, you can even declare more precisely, which parts of your ert deftest shall be protected. Furthermore, IIUC, your new macro succedds always if the form doesn't crash. So we have no information, whether the form returns nil, non-nil, or an error, in case it doesn't crash. I would still like to see the test result from should-not-crash in this case. Best regards, Michael. From debbugs-submit-bounces@debbugs.gnu.org Sat Jan 18 11:40:39 2025 Received: (at 75648) by debbugs.gnu.org; 18 Jan 2025 16:40:39 +0000 Received: from localhost ([127.0.0.1]:43519 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1tZBsF-0000iP-16 for submit@debbugs.gnu.org; Sat, 18 Jan 2025 11:40:39 -0500 Received: from mail-10628.protonmail.ch ([79.135.106.28]:61613) by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1tZBsB-0000iA-Op for 75648@debbugs.gnu.org; Sat, 18 Jan 2025 11:40:36 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1737218429; x=1737477629; bh=nm2Jhk1GLhIFi2iY+QpN4Bqj9t9vy+NTCBiHhdEsq7A=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post; b=gVvy5zEY4hogySE90RxG+i2CYHZAYHXozajzpCGg0jsjDZDrrO3YbwO36P4IV0PrM 4g+UWlDnSyr4xW/h2zkWy+lSG4pk36d04aStHt5BjxfkZXaJSV4ytjRZFnrhUmGit1 +nqRBWEMk7JS4tGI3aWyMtEj7kNAMasGINzWOhLeNiP/QSVnSoUCrD0+HNLQE7F7We 2gWNnSE/LHQt6BrLPpTAjED2ApZbOJ/4yJkfGtSoHg8HWqwN+FRIoeeIVYmNLG+isp WjdL251DP4M4+gJgyITh6XUy7cGdnyauWjEa5Cfgte17QJB69z1VuzABvLNfM3977O SOw5TaWZT51Zw== Date: Sat, 18 Jan 2025 16:40:21 +0000 To: Eli Zaretskii From: Pip Cet Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c Message-ID: <87wmesaxel.fsf@protonmail.com> In-Reply-To: <86cygk9m2x.fsf@gnu.org> References: <87jzasco3g.fsf@protonmail.com> <87cygkcifu.fsf@protonmail.com> <86cygk9m2x.fsf@gnu.org> Feedback-ID: 112775352:user:proton X-Pm-Message-ID: 266c8ed5397f86554386b487e1c67c0d4f0303c9 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 75648 Cc: Andrea Corallo , Michael Albinus , Stefan Monnier , 75648@debbugs.gnu.org, Stefan Kangas X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) "Eli Zaretskii" writes: >> Date: Sat, 18 Jan 2025 14:20:39 +0000 >> From: Pip Cet via "Bug reports for GNU Emacs, >> the Swiss army knife of text editors" >> >> "Pip Cet via \"Bug reports for GNU Emacs, the Swiss army knife of text e= ditors\"" writes: >> >> > 1. Give such tests a :crash tag >> > 2. Introduce a should-not-crash macro which succeeds if the form it >> > evaluates returns in any way, whether by error or not. >> > 3. Modify ert.el to print when a :crash test is about to start running= . >> > This allows us to identify the crashing test. >> > 4. Deviate from the current logical test order and put crash tests las= t. >> > It's conceivable that if a once-fixed crash reoccurs, the reason is a >> > simple bug that may show up in regular tests, too. If two tests fail >> > and one of them crashes the test run, it's better for the first failur= e >> > to have been reported first. >> > >> > I'll send a patch once this has a bug number. >> >> Patch 1 adds :crash tests to ert.el. > > Hmm... maybe this should be discussed separately, as it's a much > broader issue. Can you provide the motivation, both for the new macro > (which is basically a one-liner), and for the new tag? > > I've added some people to the discussion, because of this particular > part (maybe we should take this part to a separate thread). Responding separately to these two issues. If we need a new bug number, please let me know. This is the ERT response. Summary: Some bugs cause Emacs to crash. We'd like to introduce regression tests so the crashes, once fixed, won't reappear. However, if they do, ERT currently makes it hard to find out which test failed. Also, these cases are usually such strange things for Elisp to do that that it's perfectly okay for behavior to be undefined or change: we don't want to test a return value using (should) or (should-not), and we want to permit errors but not require them. >> + (cl-destructuring-bind (stats test ) event-args >> + (and (not ert-quiet) >> + (memq :crash (ert-test-tags test)) >> + (let* ((max (prin1-to-string (length (ert--stats-tests s= tats)))) >> + (format-string (concat "%9s %" >> + (prin1-to-string (length m= ax)) >> + "s/" max " %S"))) >> + (message format-string >> + "started" >> + (1+ (ert--stats-test-pos stats test)) >> + (ert-test-name test)))))) > > Is it guaranteed that this text will be output before Emacs dies? It reaches errputc, which has this code: fputc_unlocked (c, errstream ()); #ifdef WINDOWSNT /* Flush stderr after outputting a newline since stderr is fully buffered when redirected to a pipe, contrary to POSIX. */ if (c =3D=3D '\n') fflush_unlocked (stderr); #endif So I think it does (but it's different from vmessage, which calls fflush in other cases). > Maybe we should introduce flush-standard-error, or change the code to > print to stdout and use flush-standard-output? I don't think that's necessary. Pip From debbugs-submit-bounces@debbugs.gnu.org Sat Jan 18 11:44:26 2025 Received: (at 75648) by debbugs.gnu.org; 18 Jan 2025 16:44:26 +0000 Received: from localhost ([127.0.0.1]:43524 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1tZBvu-0000oa-6S for submit@debbugs.gnu.org; Sat, 18 Jan 2025 11:44:26 -0500 Received: from mail-10631.protonmail.ch ([79.135.106.31]:61713) by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1tZBvr-0000oH-Ub for 75648@debbugs.gnu.org; Sat, 18 Jan 2025 11:44:24 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1737218656; x=1737477856; bh=SpFrmSNy2StsHF/untnQ6J9SpkByJ+kR3AIxFWhbPVA=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post; b=NRItRudjDPYTExZkTJY5BoZCM7obpxdM289mqPn4zjPYAf34G3sLgkfDz9gUHX+Nm kzdGeEtlt3nVshXKjmJuqsA/xqktUFYsevRoFMLQb7HHNsd/K7Vz+Xr6miI6J7Eq4e PNp/s8mOxYVrv6vzyxNegehR4WLq7CmbJvsdjOxTHVpN6ZDYHYEYF0cayYwYRxrkXG cwtQNDBNJAAE/YizaEpNMfglCmB5qElAgFrtA79S095G3N9Uw9AZV+PkDlpPzIrVSI DcY837WOfIIv/x/fAxJva7hhuQZ0t5YI69bEzPtfbcddc5lHXOfoGrYdzCK4wLXSAQ JIGzT+H46LmiA== Date: Sat, 18 Jan 2025 16:44:12 +0000 To: Eli Zaretskii From: Pip Cet Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c Message-ID: <87sepgax84.fsf@protonmail.com> In-Reply-To: <86cygk9m2x.fsf@gnu.org> References: <87jzasco3g.fsf@protonmail.com> <87cygkcifu.fsf@protonmail.com> <86cygk9m2x.fsf@gnu.org> Feedback-ID: 112775352:user:proton X-Pm-Message-ID: 7a0eb0ebbb1d5b117eacae947d34dd2537a9ae97 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 75648 Cc: 75648@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) "Eli Zaretskii" writes: >> @@ -2697,15 +2700,14 @@ plist_put (Lisp_Object plist, Lisp_Object prop, = Lisp_Object val) >> =09 return plist; >> =09} >> >> - prev =3D tail; >> - tail =3D XCDR (tail); >> + prev_cdr =3D tail =3D tail_cdr;; >> } >> CHECK_TYPE (NILP (tail), Qplistp, plist); >> Lisp_Object newcell >> - =3D Fcons (prop, Fcons (val, NILP (prev) ? plist : XCDR (XCDR (prev= )))); >> - if (NILP (prev)) >> + =3D Fcons (prop, Fcons (val, NILP (prev_cdr) ? plist : XCDR (prev_c= dr))); >> + if (NILP (prev_cdr)) >> return newcell; >> - Fsetcdr (XCDR (prev), newcell); >> + Fsetcdr (prev_cdr, newcell); >> return plist; >> } > > This is fine by me, but I wonder whether we should replace the NILP > tests with !CONSP, when we are about to take XCAR or XCDR? (Response to the plist bug fix only, thus I reduced the CC list). We should! I missed that one. Is it okay if I just use CONSP and reverse the arguments? "CONSP (x) ? XCDR (x) : y" makes it a little more obvious that the code is safe than "!CONSP (x) ? y : XCDR (x)", IMHO. Pip From debbugs-submit-bounces@debbugs.gnu.org Sat Jan 18 12:10:17 2025 Received: (at 75648) by debbugs.gnu.org; 18 Jan 2025 17:10:17 +0000 Received: from localhost ([127.0.0.1]:43592 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1tZCKu-00029p-ND for submit@debbugs.gnu.org; Sat, 18 Jan 2025 12:10:17 -0500 Received: from mail-40133.protonmail.ch ([185.70.40.133]:39665) by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1tZCKs-00028B-78 for 75648@debbugs.gnu.org; Sat, 18 Jan 2025 12:10:14 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1737220207; x=1737479407; bh=J7RE953pVUDmMl+/0lrnDm6B09CuuA+3/o+n6JQI7Rk=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post; b=yEoCFmZ10jxSHzWD1UFjFaL9ohUFhx6BC5wwNj6qfNPAzWXFquwFDIqt6r6ZPRZxB 68BihZXM367toKWYDHCP7EbIgqEieA1yNGDIHK3W3QzH5JN2NpDn+FFaTkXUgfuZz0 B7JmJtX+QoR6zXnhnjsPWc52Sar6JI9a/iFuiw3gJAVTasvuBr+HTL3VoGEFriGhFz R+wYb1TfMydda/79dZmj7cijxz+3pXSOsjx3IXI4P1OMfFnXurGdbelLox911kIpyo W+4tDPXWHb7bV37Nib3BMfmCX6w92Xrhw+RtyeTpgqm0gkCBrh+MM8pgZ9K1vVyTBu EitTEz75PArYQ== Date: Sat, 18 Jan 2025 17:10:02 +0000 To: Michael Albinus From: Pip Cet Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c Message-ID: <87o704aw13.fsf@protonmail.com> In-Reply-To: <8734hgf5sh.fsf@gmx.de> References: <87jzasco3g.fsf@protonmail.com> <87cygkcifu.fsf@protonmail.com> <86cygk9m2x.fsf@gnu.org> <8734hgf5sh.fsf@gmx.de> Feedback-ID: 112775352:user:proton X-Pm-Message-ID: 493611a26eb0d416a002d40fc0cbd3ba925189b6 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 75648 Cc: Eli Zaretskii , Andrea Corallo , 75648@debbugs.gnu.org, Stefan Monnier , Stefan Kangas X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) "Michael Albinus" writes: > Eli Zaretskii writes: > > Hi, > >>> Patch 1 adds :crash tests to ert.el. >> >> Hmm... maybe this should be discussed separately, as it's a much >> broader issue. Can you provide the motivation, both for the new macro >> (which is basically a one-liner), and for the new tag? > > This patch adds the :crash tag to ert.el. This is unfortune. Until now, > there are no predefined tags in ert.el. Do we want to change this? It's pretty clear you don't, so let's see what else we can do: should-not-crash can print a message, but needs to somehow be able to access the ert test for that message to make sense. My naive idea is to use a dynamic let-binding for a function to (fun)call to print a message, would that work? We can't do what ert-fail does (throws a signal), IIUC, but we can make it look like an ert-fail call. > If we need such a property for ert tests, we should use another > mechanism. For example, another slot in the defstruct ert-test, like > (should-not-crash nil). And your new macro should-not-crash sets this > slot to non-nil in its scope. With this, you can even declare more > precisely, which parts of your ert deftest shall be protected. Hmm. I don't see how that would work. I'll have to study the code some more. > Furthermore, IIUC, your new macro succedds always if the form doesn't > crash. So we have no information, whether the form returns nil, non-nil, > or an error, in case it doesn't crash. I would still like to see the > test result from should-not-crash in this case. In this case, that's intentional: it's perfectly okay to do anything but crash or infloop, and we really don't want to have to update the tests for every behavior change. I haven't thought much about the case where we want to assert both that a form doesn't crash and which value it has. My intuition is that it's best to have separate tests. Admittedly that's a little easier if should-not-crash returns the value of the form. Pip From debbugs-submit-bounces@debbugs.gnu.org Sat Jan 18 13:43:12 2025 Received: (at 75648) by debbugs.gnu.org; 18 Jan 2025 18:43:12 +0000 Received: from localhost ([127.0.0.1]:43723 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1tZDmq-0006Mf-1J for submit@debbugs.gnu.org; Sat, 18 Jan 2025 13:43:12 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:52784) by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1tZDmm-0006MM-KE for 75648@debbugs.gnu.org; Sat, 18 Jan 2025 13:43:09 -0500 Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1tZDmh-0006Ld-BV; Sat, 18 Jan 2025 13:43:03 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=SHWXmOxA9/HZ4ClcppgJ/h1iBgyNme1Ii6eBkerOWh8=; b=OJLgnyAkODhd ho+TJnbNXzAQ8yQfOk3NEmFJBog5ZnZ5Y7YCcd/asGJSy6CyidJEBfnZDFS7rZqQMaj2W2XJ1cuhm 8KfvFDfLyRSQampZRVSW+axhwBm8TAlKY59kJceXsMCvsyCoe3v58jlf1k4A8Lyb5NpbFiXsUdV+q fphSrULDrdOYcYaXixbcPvzC+Fztdk2qRtzuf50Z/UZqjALGdYIW3iL3T+aFPhDQGhXiLraXwnxl/ U1GMp28pn5dt17m/GN6rw80YiffoIxsIO3juBdKA+lzBb/Z5XaYWKBCF3sAl8pgyEelyTRHwoNzmI xWjR5alvvDc4wQAKN4hkKA==; Date: Sat, 18 Jan 2025 20:43:01 +0200 Message-Id: <865xmc9d56.fsf@gnu.org> From: Eli Zaretskii To: Pip Cet In-Reply-To: <87sepgax84.fsf@protonmail.com> (message from Pip Cet on Sat, 18 Jan 2025 16:44:12 +0000) Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c References: <87jzasco3g.fsf@protonmail.com> <87cygkcifu.fsf@protonmail.com> <86cygk9m2x.fsf@gnu.org> <87sepgax84.fsf@protonmail.com> X-Spam-Score: -2.3 (--) X-Debbugs-Envelope-To: 75648 Cc: 75648@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -3.3 (---) > Date: Sat, 18 Jan 2025 16:44:12 +0000 > From: Pip Cet > Cc: 75648@debbugs.gnu.org > > "Eli Zaretskii" writes: > > > This is fine by me, but I wonder whether we should replace the NILP > > tests with !CONSP, when we are about to take XCAR or XCDR? > > (Response to the plist bug fix only, thus I reduced the CC list). > > We should! I missed that one. Is it okay if I just use CONSP and > reverse the arguments? "CONSP (x) ? XCDR (x) : y" makes it a little more > obvious that the code is safe than "!CONSP (x) ? y : XCDR (x)", IMHO. Sure, that's okay. From debbugs-submit-bounces@debbugs.gnu.org Sat Jan 18 18:31:04 2025 Received: (at 75648) by debbugs.gnu.org; 18 Jan 2025 23:31:04 +0000 Received: from localhost ([127.0.0.1]:44124 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1tZIHP-0006qu-ST for submit@debbugs.gnu.org; Sat, 18 Jan 2025 18:31:04 -0500 Received: from mail-ed1-x535.google.com ([2a00:1450:4864:20::535]:61574) by debbugs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1tZIHN-0006qF-C6 for 75648@debbugs.gnu.org; Sat, 18 Jan 2025 18:31:02 -0500 Received: by mail-ed1-x535.google.com with SMTP id 4fb4d7f45d1cf-5d3f28a4fccso4965766a12.2 for <75648@debbugs.gnu.org>; Sat, 18 Jan 2025 15:31:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1737243055; x=1737847855; darn=debbugs.gnu.org; h=cc:to:subject:message-id:date:mime-version:references:in-reply-to :from:from:to:cc:subject:date:message-id:reply-to; bh=a46bR4ihTOImdRMzu82sPoEh56z7zJ3xr9UsaLbkv3c=; b=Vgr6Jx2YchUEFKPFOJpA5AHBvBJeNjtD4dWiWGxYxL0Cg+o92mcOlQqxsrPGm1Kf0/ K28ipgwFAsGkYQQl57uU53sVu3gF2ST2dYn+RlIcLS8P2UH9VkTdttU5+y0ezfatGwfr uLA5zkwS3ieOYrH4l3Joks00T9LrR5sZ0w0ILS5Go9EV2w0Zmc2QFTxC1OZFMnLgtVi9 66io65nOkx7+uc7oW7D7C2aUDLi2l2WNSRteImZ81cnPxe7pSGHPgE150+pvtjtGJqHl VGmwchh9T0ugveeMKdm5XIR30E/qZPn9FXDyUlxMgryOBbPosEpL06ZpcDbQhyuC5yeA Pkcw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737243055; x=1737847855; h=cc:to:subject:message-id:date:mime-version:references:in-reply-to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=a46bR4ihTOImdRMzu82sPoEh56z7zJ3xr9UsaLbkv3c=; b=FJq6RbTeRwZG47KJYvzetmJO77nBK39FdYlqwGb5r0R0HoaUjBMe44UIPK/ax2chh7 pKoKZv0rzSz/v2SGS+QYXGkL8/T//775sj7ddaX6+kuy4WBd2S4Z6pUroctAWFhuBcXb DeANXTxaaFT1DansTYZI1KonQcEdi6ohb0oI4fTGOn/rhvQr4XSSZJwldug66pd6MJac fuY6Mn6zEayLU5FwxSp1QqUWCqWyREPZHAWcQIbpO36qMf3UQe2bHwCeFqw9czCI18xC K+UbNX9jf6D9XjUy27R8RgQGM5ZrdQHvb1bd/v5ebDSZlPVP14nQqJv5X7qjHIm3T2aD F0vA== X-Forwarded-Encrypted: i=1; AJvYcCU76TEel0D2EBtmgmxRzJoNXLQOYQgX1w86/DE2UmXRFnM19yJLM1hwDJUtijHkfIVGMpT7VQ==@debbugs.gnu.org X-Gm-Message-State: AOJu0Yw9k/RN7GXOKaSxBvgoVizElXAAnLxdHAKWNVBUoJXhh3fXjyiV 7Sr6L6g+0+Z1VftQYuym3owdRAvloZ2hC0ErjwOQccJsV3iwWktSwLFWtmbFuq+uhdE9aJBNB+7 7gjCl1T/HqiEg5jWoqy3AwDz+ZVQ= X-Gm-Gg: ASbGnct1ROZNtiPe8A6pshzccAsGYIyyU05rfLK7Zri6NFZjBRYKrvQ+7wVmp2Rl9vL JLu5dpI2jBB4z2QcqUPszAizaU/E4l+VaL+xGQmKtgVjW4/naj15B X-Google-Smtp-Source: AGHT+IEM9VzBC27Gys9os1A7WH9MaMCo0HROp/7uhcqwElShrtkG1mnAbNP1+QCMoZtmBoGtDH3udrb9TLVIADaDhJM= X-Received: by 2002:a05:6402:3487:b0:5d4:1ac2:277f with SMTP id 4fb4d7f45d1cf-5db7d2f5efdmr5956713a12.9.1737243054933; Sat, 18 Jan 2025 15:30:54 -0800 (PST) Received: from 753933720722 named unknown by gmailapi.google.com with HTTPREST; Sat, 18 Jan 2025 17:30:54 -0600 From: Stefan Kangas In-Reply-To: <87wmesaxel.fsf@protonmail.com> References: <87jzasco3g.fsf@protonmail.com> <87cygkcifu.fsf@protonmail.com> <86cygk9m2x.fsf@gnu.org> <87wmesaxel.fsf@protonmail.com> MIME-Version: 1.0 Date: Sat, 18 Jan 2025 17:30:54 -0600 X-Gm-Features: AbW1kvbrBs5oyB3dSd49N1SYBTtzXVdeaSZZnhZ331GkSrSJR0lMncxXuVY6guI Message-ID: Subject: Re: bug#75648: Minor safety improvements to fns.c/eval.c To: Pip Cet , Eli Zaretskii Content-Type: text/plain; charset="UTF-8" X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 75648 Cc: Andrea Corallo , Michael Albinus , Stefan Monnier , 75648@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.0 (-) Pip Cet writes: > "Eli Zaretskii" writes: > >>> Patch 1 adds :crash tests to ert.el. >> >> Hmm... maybe this should be discussed separately, as it's a much >> broader issue. Can you provide the motivation, both for the new macro >> (which is basically a one-liner), and for the new tag? >> >> I've added some people to the discussion, because of this particular >> part (maybe we should take this part to a separate thread). > > Responding separately to these two issues. If we need a new bug number, > please let me know. This is the ERT response. > > Summary: Some bugs cause Emacs to crash. We'd like to introduce > regression tests so the crashes, once fixed, won't reappear. However, > if they do, ERT currently makes it hard to find out which test failed. > Also, these cases are usually such strange things for Elisp to do that > that it's perfectly okay for behavior to be undefined or change: we > don't want to test a return value using (should) or (should-not), and we > want to permit errors but not require them. I assume that the justification for the new :crash property is improved display of the test result. Is that right? If so, I think it would be useful to see an example excerpt of the output in case a test crashes Emacs before and after this patch. The patch that fixes the bug LGTM, but I see that Eli had a suggestion for how to improve things even further. Thanks for working on this.