* bug#46534: Lexical change in bindat breaks weechat.el
@ 2021-02-15 14:29 Tony Olagbaiye
2021-02-15 15:50 ` Stefan Monnier
0 siblings, 1 reply; 6+ messages in thread
From: Tony Olagbaiye @ 2021-02-15 14:29 UTC (permalink / raw)
To: 46534; +Cc: monnier
[-- Attachment #1.1.1: Type: text/plain, Size: 1380 bytes --]
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
[-- Attachment #1.1.2.1: Type: text/html, Size: 1364 bytes --]
[-- Attachment #1.2: weechat-bindat.bug.el.txt --]
[-- Type: text/plain, Size: 2745 bytes --]
;;; Adapted from https://github.com/the-kenny/weechat.el/blob/master/weechat-relay.el
;(load-file "/nix/store/vy6lzg2b2kmsm4yyl7x3j9pzy10y3bsv-emacs-weechat-20190520.1551/share/emacs/site-lisp/elpa/weechat-20190520.1551/weechat.el")
;(weechat-unpack-message "\0\0\0\"\0\0\0\0\2G0inf\0\0\0\7version\0\0\0\0053.0.1")
(require 'bindat)
(require 'cl-lib)
(setq debug-on-error t)
(defun minrepro--unpack-str (data)
"Unpacks a weechat-relay-string from unibyte string DATA.
Optional second return value contains length of parsed data."
(let ((obj (bindat-unpack minrepro--str-spec data)))
(cl-values (decode-coding-string (bindat-get-field obj 'val) 'utf-8)
(bindat-length minrepro--str-spec obj))))
(defun minrepro--parse-inf (data)
(cl-multiple-value-bind (name len) (minrepro--unpack-str data)
(cl-multiple-value-bind (value len*) (minrepro--unpack-str (substring data len))
(cl-values (cons name value)
(+ len len*)))))
(defun minrepro--bindat-unsigned-to-signed (num bytes)
"Convert an unsigned int NUM to signed int.
NUM is in two-complement representation with BYTES bytes.
Useful because bindat does not support signed numbers."
(if (> num (- (expt 2 (- (* 8 bytes) 1)) 1))
(- num (expt 2 (* 8 bytes)))
num))
(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))))))
(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)))))
(defun minrepro--unpack-message-contents (data)
(let* ((type (substring data 0 3))
(fun (symbol-function (intern (concat "minrepro--parse-" type)))))
(cl-multiple-value-bind (obj len) (funcall fun (string-make-unibyte (substring data 3)))
(cl-values obj
(+ len 3)))))
(let* ((msg (bindat-unpack minrepro--message-spec "\0\0\0\"\0\0\0\0\2G0inf\0\0\0\7version\0\0\0\0053.0.1"))
(data (concat (bindat-get-field msg 'data)))
(msg-id (bindat-get-field msg 'id 'val))
(offset 0)
(acc ()))
;; Only no-compression is supported atm
(unless (= 0 (bindat-get-field msg 'compression))
(error "Compression not supported"))
(while (< offset (length data))
(cl-multiple-value-bind (obj offset*) (minrepro--unpack-message-contents
(substring data offset))
(setq offset (+ offset offset*))
(setq acc (cons obj acc))))
(cl-values (cons msg-id (reverse acc))
(bindat-get-field msg 'length)))
[-- Attachment #1.3: publickey - EmailAddress(s=me@fron.io) - 0x3026807C.asc --]
[-- Type: application/pgp-keys, Size: 616 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 233 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#46534: Lexical change in bindat breaks weechat.el
2021-02-15 14:29 bug#46534: Lexical change in bindat breaks weechat.el Tony Olagbaiye
@ 2021-02-15 15:50 ` Stefan Monnier
2021-02-15 19:19 ` Tony Olagbaiye
2021-02-17 22:47 ` Kim Storm
0 siblings, 2 replies; 6+ messages in thread
From: Stefan Monnier @ 2021-02-15 15:50 UTC (permalink / raw)
To: Tony Olagbaiye; +Cc: 46534, Kim F. Storm
[ 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))
^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#46534: Lexical change in bindat breaks weechat.el
2021-02-15 15:50 ` Stefan Monnier
@ 2021-02-15 19:19 ` Tony Olagbaiye
2021-02-15 19:51 ` Stefan Monnier
2021-02-17 22:47 ` Kim Storm
1 sibling, 1 reply; 6+ messages in thread
From: Tony Olagbaiye @ 2021-02-15 19:19 UTC (permalink / raw)
To: monnier; +Cc: 46534, storm
[-- Attachment #1.1.1: Type: text/plain, Size: 4282 bytes --]
Indeed, I can confirm this fixes the test file as well as weechat.el proper, thanks!
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))
[-- Attachment #1.1.2.1: Type: text/html, Size: 4839 bytes --]
[-- Attachment #1.2: publickey - EmailAddress(s=me@fron.io) - 0x3026807C.asc --]
[-- Type: application/pgp-keys, Size: 616 bytes --]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 233 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#46534: Lexical change in bindat breaks weechat.el
2021-02-15 19:19 ` Tony Olagbaiye
@ 2021-02-15 19:51 ` Stefan Monnier
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Monnier @ 2021-02-15 19:51 UTC (permalink / raw)
To: Tony Olagbaiye; +Cc: 46534-done, storm
> 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))
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#46534: Lexical change in bindat breaks weechat.el
2021-02-15 15:50 ` Stefan Monnier
2021-02-15 19:19 ` Tony Olagbaiye
@ 2021-02-17 22:47 ` Kim Storm
2021-02-18 16:09 ` Stefan Monnier
1 sibling, 1 reply; 6+ messages in thread
From: Kim Storm @ 2021-02-17 22:47 UTC (permalink / raw)
To: Stefan Monnier, Tony Olagbaiye; +Cc: 46534
On 15/02/2021 16.50, Stefan Monnier wrote:
> [ Hi Kim, long time no see.
> I'd appreciate your opinion on this issue with bindat.el. ]
Hi Stefan
Indeed, it's been a while since I wrote that code :-)
Your change seems to be the simplest way to solve the "not last field"
issue with "eval".
BTW, the following line seems wrong:
;; (length u16r) ;; little endian order
Since the C struct specifies "unsigned long", it should be u32r rather
than u16r.
Also the C code samples should use uint8_t uint16_t and uint32_t for
clarity.
Kim
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#46534: Lexical change in bindat breaks weechat.el
2021-02-17 22:47 ` Kim Storm
@ 2021-02-18 16:09 ` Stefan Monnier
0 siblings, 0 replies; 6+ messages in thread
From: Stefan Monnier @ 2021-02-18 16:09 UTC (permalink / raw)
To: Kim Storm; +Cc: 46534, Tony Olagbaiye
>> [ Hi Kim, long time no see.
>> I'd appreciate your opinion on this issue with bindat.el. ]
> Indeed, it's been a while since I wrote that code :-)
As you can see, it's still alive and kicking.
> Your change seems to be the simplest way to solve the "not last field"
> issue with "eval".
Thanks for confirming.
> BTW, the following line seems wrong:
>
> ;; (length u16r) ;; little endian order
>
> Since the C struct specifies "unsigned long", it should be u32r rather
> than u16r.
Good catch.
> Also the C code samples should use uint8_t uint16_t and uint32_t
> for clarity.
Oh, yes, thanks,
Stefan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-18 16:09 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-15 14:29 bug#46534: Lexical change in bindat breaks weechat.el Tony Olagbaiye
2021-02-15 15:50 ` Stefan Monnier
2021-02-15 19:19 ` Tony Olagbaiye
2021-02-15 19:51 ` Stefan Monnier
2021-02-17 22:47 ` Kim Storm
2021-02-18 16:09 ` Stefan Monnier
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).