* 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).