From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Philipp Stephani Newsgroups: gmane.emacs.bugs Subject: bug#41988: 28.0.50; Edebug unconditionally instruments definitions with &define specs Date: Mon, 10 Aug 2020 15:32:22 +0200 Message-ID: References: <20200621234816.88427.qmail@mail.muc.de> <20200808145948.GA10181@ACM> <20200809163523.GB26635@ACM> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="31678"; mail-complaints-to="usenet@ciao.gmane.io" Cc: 41988@debbugs.gnu.org To: Alan Mackenzie Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Mon Aug 10 15:43:16 2020 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 1k5856-00087g-Gt for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 10 Aug 2020 15:43:16 +0200 Original-Received: from localhost ([::1]:37624 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1k5855-0003ue-Jy for geb-bug-gnu-emacs@m.gmane-mx.org; Mon, 10 Aug 2020 09:43:15 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:47800) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1k57vC-00064p-2k for bug-gnu-emacs@gnu.org; Mon, 10 Aug 2020 09:33:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:51648) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1k57vB-0000gr-Po for bug-gnu-emacs@gnu.org; Mon, 10 Aug 2020 09:33:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1k57vB-0003vf-ND for bug-gnu-emacs@gnu.org; Mon, 10 Aug 2020 09:33:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Philipp Stephani Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Mon, 10 Aug 2020 13:33:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 41988 X-GNU-PR-Package: emacs Original-Received: via spool by 41988-submit@debbugs.gnu.org id=B41988.159706636214707 (code B ref 41988); Mon, 10 Aug 2020 13:33:01 +0000 Original-Received: (at 41988) by debbugs.gnu.org; 10 Aug 2020 13:32:42 +0000 Original-Received: from localhost ([127.0.0.1]:34961 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1k57us-0003ok-7y for submit@debbugs.gnu.org; Mon, 10 Aug 2020 09:32:42 -0400 Original-Received: from mail-ot1-f41.google.com ([209.85.210.41]:39452) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1k57up-0003jR-M6 for 41988@debbugs.gnu.org; Mon, 10 Aug 2020 09:32:40 -0400 Original-Received: by mail-ot1-f41.google.com with SMTP id z18so7243010otk.6 for <41988@debbugs.gnu.org>; Mon, 10 Aug 2020 06:32:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=a46mJzD7MlT4mJ6NWGxSDXsjJv17kGZi3yiLR7mhIio=; b=tj1EoL2ca5S/3UyGVgnKlxF21IBvalss60EcZIOhJEJt5S8Bqj0t3/XEHGM8Bb6s06 He23o92FhJBREYsDyI4NQ/i13XBbU/1ozMb+6Yx2OuuSysOwOJrjze5ywupMkFCsJ9Kl KHGozTWagc+tX931RM3GVst5JU/zh5j5xks74LPOCcqyIXnObU0alsx7u0ogLiRTzyS4 dY9cK0zR+HC6CbbvBAQwRboYZHXdZYnXdQdkAa5MdkzZ8au4YlFr/Iltb+I/ZUqQdgA2 mLvl9gFkbYW/9dSSR92igcY9KX+B9Qu8W57m9VrsGjUT3FuMK1v5d+onhL/k/uziq12s BgKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=a46mJzD7MlT4mJ6NWGxSDXsjJv17kGZi3yiLR7mhIio=; b=peVZ+AKW4rI+Ihf4Eu8PMhU9fTdUPRUeuXoW0qnkp2wLo4HQmg40P9MMgGpcxQiWpY TUR5l0TF3k3/LHV2QDm5lfC3J/toZ6Z3eAMhrh9HHcO3+MGBiuOA3C8gWXuU5SpBaay7 7yHy0tRMm13Ae2uo/CwOz9MzEethX34eWCVXyqQ7/DEU9IHK0WEBjakWNknK0IxK7t8p wSGkAakb0O7CsA6EesBO80xY4CWjI/1w6no7kaWmm/LWYdGcK/nYjksHi+PBMao4q31P /A2smSuualnJJMjDJL1Tyj0CMxUKO6BXCUtP1NXeAIiufpLJntXshuubdLNbSNlfeBBQ PVaQ== X-Gm-Message-State: AOAM533khhjFRbkpGSX+uSfGystjD30jTqGKv7xvZMOWx4BH+cPIeTC/ KasPqYuaktR0tYcqtow6Ftg19vyISA0i5Hl6xIptT7HS X-Google-Smtp-Source: ABdhPJz9Pct3+3mAXlvEaK1CQEbkhEwgFLXDZjpCzasffSEmRDeQzDzArsPVaOJuFkvYLYlOq3rnom94FWZWesfRG7s= X-Received: by 2002:a9d:2203:: with SMTP id o3mr753633ota.149.1597066353477; Mon, 10 Aug 2020 06:32:33 -0700 (PDT) In-Reply-To: <20200809163523.GB26635@ACM> 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" Xref: news.gmane.io gmane.emacs.bugs:184509 Archived-At: Am So., 9. Aug. 2020 um 18:35 Uhr schrieb Alan Mackenzie : > > Hello, Philipp. > > On Sun, Aug 09, 2020 at 13:33:53 +0200, Philipp Stephani wrote: > > Am Sa., 8. Aug. 2020 um 16:59 Uhr schrieb Alan Mackenzie : > > > > I must admit, I'm having difficulty understanding this problem. > > > > On Sat, Aug 08, 2020 at 13:01:50 +0200, Philipp Stephani wrote: > > > > Am Mo., 22. Juni 2020 um 01:48 Uhr schrieb Alan Mackenzie : > > [ .... ] > > > > > So this is somewhat subtle, so let me try to give some context. The > > > > message is merely a symptom of defining a symbol twice (via > > > > edebug-make-form-wrapper). That's a problem when using Edebug for > > > > coverage instrumentation (in batch mode), as the coverage > > > > information is attached to properties of the symbol that Edebug > > > > generates/instruments. > > > > I'm trying to see what, exactly, this problem is. Edebug is defining > > > a symbol twice, once for each of two arms of a &or form in the edebug > > > spec. The first of these surely does nothing; it will eventually end > > > up in the garbage collector. The second will form the function slot > > > of the symbol, fulfilling all the Edebug things. What am I missing? > > > The problem is that Edebug not only generates objects that would later > > be garbage-collected (and therefore not observable), but also modifies > > observable global state. This starts at > > https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/edebug.el?id=55bcb3f7e05c01d86778f1a2b7caccf72124614d#n1418 > > and continues for the rest of the edebug-make-form-wrapper function. > > In particular, > > https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/edebug.el?id=55bcb3f7e05c01d86778f1a2b7caccf72124614d#n1444 > > sets the `edebug' symbol property of the symbol being generated. None > > of these mutations are undone when backtracking. > > Ah, I think I see it, now. edebug-form-data contains structures > referring to functions, and could well have two entries with the same > function name. (I see that at the moment in a file where I instrumented > alternately an old version and a new version of a function.) The > property list on the symbol then contains a messy combination of details > for the two functions. Yes. > > > > > Instrumenting a symbol with two different definitions can lead to > > > > very subtle bugs because the frequency vector and the form offset > > > > vector are out of sync, .... > > > > The picture you seem to be painting is of two distinct definitions > > > being assigned to the same symbol, and both of them being live. Do > > > you have any evidence that this is happening? > > > Let's say it's rather an incompatible mixture of two definitions. > > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853 is a symptom of > > this. Another piece of evidence is the implementation of > > `edebug-make-form-wrapper': that function clearly modifies buffer > > contents and symbol properties even in branches that would later be > > discarded as part of backtracking. > > My (not well evidenced) assumption is that > > https://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/emacs-lisp/edebug.el?id=55bcb3f7e05c01d86778f1a2b7caccf72124614d#n1427 > > regenerates the offset vector, but there's no regeneration of the > > frequency vector, which is the immediate trigger of > > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853, since now the > > frequency and offset vectors might be incompatible with each other. > > But I'd also assume the problem runs deeper: edebug-make-form-wrapper > > performs multiple mutations, and it's not really clear which of those > > are "safe" w.r.t. multiple definitions in not-taken branches. > > How about, instead of having symbol properties, we institute non-symbol > property lists contained in each entry in edebug-form-data? This list > could be rapidly searched, with repeated memq, for the pertinent entry. > It would mean, however, that all gathered data would be discarded on each > fresh instrumentation of a function. Apologies if you've already > suggested this and I missed it. That seems roughly what the documentation of `edebug-form-data' itself suggests: "In the future (haha!), the symbol will be irrelevant and edebug data will be stored in the definitions themselves rather than in the property list of a symbol." Though I don't know what "the definitions themselves" is supposed to mean here. In any case, I'd agree that attaching the data (in an undocumented manner) to some symbol that then carefully needs to be constructed to be unique is too brittle. However, the current implementation is used in quite a few places (see e.g. the interactive specification of `edebug-remove-instrumentation'), so changing it isn't trivial. > > > > > .... see e.g. https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853. > > > > Therefore it's important to prevent such duplicate instrumentation, > > > > typically by changing the Edebug symbol in some way (appending a unique > > > > suffix, etc.). Edebug does this already in many cases (ERT tests, CL > > > > methods, ...), but not always. For some more context, see the coverage > > > > instrumentation in my Bazel rules for ELisp > > > > (https://github.com/phst/rules_elisp). > > > > https://github.com/phst/rules_elisp/blob/master/elisp/ert/runner.el > > > > contains the ERT and coverage integration. In > > > > https://github.com/phst/rules_elisp/blob/0b24aa1660af2f6c668899bdd78aaba383d7ac18/elisp/ert/runner.el#L133-L134 > > > > I explicitly check for duplicate instrumentation. It is hard to predict > > > > in general whether a specific instance of duplicate instrumentation > > > > will lead to bugs like > > > > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=41853 or not, thus I'm > > > > treating every duplicate instrumentation as a bug. > > > > What exactly do you mean by "duplicate instrumentation"? If a symbol > > > gets defined twice, once for each arm of an &or in the edebug spec, does > > > that count as a duplicate instrumentation? > > > What I mean concretely is evaluating `edebug-make-form-wrapper' (and > > therefore, mutating symbol properties and buffer contents) once for > > each branch of an &or construct. > > OK, thanks. How does my above plan, for reinitilising the function's > properties at each instrumentation, sound? It would definitely be an improvement I think.