--- Begin Message ---
Subject: |
Lexical change in bindat breaks weechat.el |
Date: |
Mon, 15 Feb 2021 14:29:06 +0000 |
Hi,
As of commit c8c4d65d6510724acd40527a9af67e21e3cf4d5e (as bisected in my stead by wasamasa on #emacs) it seems bindat changes have broken weechat.el, or specifically the weechat-relay module.
Attached a minimal reproducible script which fails on master but succeeds prior to the mentioned commit: weechat-bindat.bug.el.txt
Backtrace:
Debugger entered--Lisp error: (void-variable struct) (bindat-get-field struct 'len) (weechat--relay-bindat-unsigned-to-signed (bindat-get-field struct 'len) 4) (let ((len (weechat--relay-bindat-unsigned-to-signed (bindat-get-field struct 'len) 4))) (if (<= len 0) 0 len)) bindat--unpack-group(((len u32) (val str (eval (let ((len (weechat--relay-bindat-unsigned-to-signed ... 4))) (if (<= len 0) 0 len)))))) bindat--unpack-group(((length u32) (compression u8) (id struct weechat--relay-str-spec) (data vec (eval (let ((l (- ... 4 1 ...))) l))))) bindat-unpack(((length u32) (compression u8) (id struct weechat--relay-str-spec) (data vec (eval (let ((l (- ... 4 1 ...))) l)))) "\0\0\0$\0\0\0\0\4G255inf\0\0\0\7version\0\0\0\0053.0.1") weechat-unpack-message("\0\0\0$\0\0\0\0\4G255inf\0\0\0\7version\0\0\0\0053.0.1") weechat--relay-parse-new-message() weechat--relay-process-filter(#<process weechat-relay-tls> "\0\0\0$\0\0\0\0\4G255inf\0\0\0\7version\0\0\0\0053.0.1")
Best,
bqv
weechat-bindat.bug.el.txt
Description: Text document
publickey - EmailAddress(s=me@fron.io) - 0x3026807C.asc
Description: application/pgp-keys
signature.asc
Description: OpenPGP digital signature
--- End Message ---
--- Begin Message ---
Subject: |
Re: bug#46534: Lexical change in bindat breaks weechat.el |
Date: |
Mon, 15 Feb 2021 14:51:29 -0500 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.0.50 (gnu/linux) |
> Indeed, I can confirm this fixes the test file as well as weechat.el proper,
> thanks!
Thanks, closing for now, tho I'd love to see a better fix
(e.g. changing the system so it doesn't rely on `eval` and dynamic
scoping so much).
Stefan
> Sent from ProtonMail mobile
>
> -------- Original Message --------
> On 15 Feb 2021, 15:50, Stefan Monnier < monnier@iro.umontreal.ca> wrote:
>
> [ Hi Kim, long time no see.
> I'd appreciate your opinion on this issue with bindat.el. ]
>
> > (defconst minrepro--str-spec
> > '((len u32)
> > (val str (eval (let ((len (minrepro--bindat-unsigned-to-signed
> > (bindat-get-field struct 'len)
> > 4)))
> > ;; Hack for signed/unsigned problems
> > (if (<= len 0) 0 len))))))
>
> Hmm... the doc of bindat.el does not include `struct` among the vars you
> can use in `eval`.
>
> OTOH, a variable which you can use is `last` and it indeed contains
> exactly the info you need from `struct`, so you can rewrite the above to:
>
> (defconst minrepro--str-spec
> '((len u32)
> (val str (eval (let ((len (minrepro--bindat-unsigned-to-signed
> last 4)))
> ;; Hack for signed/unsigned problems
> (if (<= len 0) 0 len))))))
>
> > (defconst minrepro--message-spec
> > '((length u32)
> > (compression u8)
> > (id struct minrepro--str-spec)
> > (data vec (eval (let ((l (- (bindat-get-field struct 'length)
> > 4 ;length
> > 1 ;compression
> > (+ 4 (length (bindat-get-field struct 'id 'val))))))
> > l)))))
>
> This one OTOH can't just use `last` since that only gives us the `id`
> field but not the `length` field :-(
>
> I can't see any way to do what you want given the documentation found in
> the `Commentary:` of `bindat.el`, so I guess we do need to extend the
> documented functionality.
>
> I installed the patch below, for now. It fixes the problem in your test
> case and hopefully in other cases as well. Please confirm.
>
> Stefan
>
> diff --git a/lisp/emacs-lisp/bindat.el b/lisp/emacs-lisp/bindat.el
> index 0d9ba57d66..bf01347ae0 100644
> --- a/lisp/emacs-lisp/bindat.el
> +++ b/lisp/emacs-lisp/bindat.el
> @@ -26,7 +26,7 @@
> ;; Packing and unpacking of (binary) data structures.
> ;;
> ;; The data formats used in binary files and network protocols are
> -;; often structed data which can be described by a C-style structure
> +;; often structured data which can be described by a C-style structure
> ;; such as the one shown below. Using the bindat package, decoding
> ;; and encoding binary data formats like these is made simple using a
> ;; structure specification which closely resembles the C style
> @@ -135,7 +135,8 @@
> ;; | ( [FIELD] repeat COUNT ITEM... )
>
> ;; -- In (eval EXPR), the value of the last field is available in
> -;; the dynamically bound variable `last'.
> +;; the dynamically bound variable `last' and all the previous
> +;; ones in the variable `struct'.
>
> ;; TYPE ::= ( eval EXPR ) -- interpret result as TYPE
> ;; | u8 | byte -- length 1
> @@ -191,7 +192,7 @@
> ;;; Code:
>
> ;; Helper functions for structure unpacking.
> -;; Relies on dynamic binding of BINDAT-RAW and BINDAT-IDX
> +;; Relies on dynamic binding of `bindat-raw' and `bindat-idx'.
>
> (defvar bindat-raw)
> (defvar bindat-idx)
> @@ -276,8 +277,8 @@ bindat--unpack-item
> (t nil)))
>
> (defun bindat--unpack-group (spec)
> - (with-suppressed-warnings ((lexical last))
> - (defvar last))
> + (with-suppressed-warnings ((lexical struct last))
> + (defvar struct) (defvar last))
> (let (struct last)
> (while spec
> (let* ((item (car spec))
> @@ -378,9 +379,9 @@ bindat--fixed-length-alist
> (ip . 4)))
>
> (defun bindat--length-group (struct spec)
> - (with-suppressed-warnings ((lexical last))
> - (defvar last))
> - (let (last)
> + (with-suppressed-warnings ((lexical struct last))
> + (defvar struct) (defvar last))
> + (let ((struct struct) last)
> (while spec
> (let* ((item (car spec))
> (field (car item))
> @@ -544,9 +545,9 @@ bindat--pack-item
> (setq bindat-idx (+ bindat-idx len)))))
>
> (defun bindat--pack-group (struct spec)
> - (with-suppressed-warnings ((lexical last))
> - (defvar last))
> - (let (last)
> + (with-suppressed-warnings ((lexical struct last))
> + (defvar struct) (defvar last))
> + (let ((struct struct) last)
> (while spec
> (let* ((item (car spec))
> (field (car item))
--- End Message ---