bug#36740: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)

bug#36740: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)

[Pip Cet]

This code in ccl.c

        eop = hash_lookup (h, make_fixnum (reg[RRR]), NULL);
        if (eop >= 0)
          {
            Lisp_Object opl;
            opl = HASH_VALUE (h, eop);
            if (! (IN_INT_RANGE (eop) && CHARACTERP (opl)))
              CCL_INVALID_CMD;
            reg[RRR] = charset_unicode;
            reg[rrr] = eop;
            reg[7] = 1; /* r7 true for success */
          }
        else
          reg[7] = 0;

seems wrong to me. We look up the hash value for reg[RRR], but then we
store the hash _index_ into reg[rrr], and throw away the actual value.

The bug appears to be present in:

commit d325055a00e658a38c1721fcc63ed1775dd8ccb3
Author: Dave Love <fx@gnu.org>
Date:   Tue Jul 30 11:31:54 2002 +0000

which added the code, so I'm not sure whether there's external code
which might rely on the buggy behavior (unlikely, I think). Is there
any code actually making use of this CCL feature?

I'm playing around with hash tables and it would help to resolve this first.

bug#36740: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Sat, 20 Jul 2019 12:29:57 +0000
> 
> This code in ccl.c
> 
>         eop = hash_lookup (h, make_fixnum (reg[RRR]), NULL);
>         if (eop >= 0)
>           {
>             Lisp_Object opl;
>             opl = HASH_VALUE (h, eop);
>             if (! (IN_INT_RANGE (eop) && CHARACTERP (opl)))
>               CCL_INVALID_CMD;
>             reg[RRR] = charset_unicode;
>             reg[rrr] = eop;
>             reg[7] = 1; /* r7 true for success */
>           }
>         else
>           reg[7] = 0;
> 
> seems wrong to me. We look up the hash value for reg[RRR], but then we
> store the hash _index_ into reg[rrr], and throw away the actual value.

The comment for the op-code says:

  #define CCL_LookupIntConstTbl 0x13 /* Lookup multibyte character by
					integer key.  Afterwards R7 set
					to 1 if lookup succeeded.
					1:ExtendedCOMMNDRrrRRRXXXXXXXX
					2:ARGUMENT(Hash table ID) */

so there appears to be no significance to r7's value?

Why did you think this code was wrong?  And why is this important in
the context of your playing with hash tables?

> The bug appears to be present in:
> 
> commit d325055a00e658a38c1721fcc63ed1775dd8ccb3
> Author: Dave Love <fx@gnu.org>
> Date:   Tue Jul 30 11:31:54 2002 +0000
> 
> which added the code, so I'm not sure whether there's external code
> which might rely on the buggy behavior (unlikely, I think). Is there
> any code actually making use of this CCL feature?

I don't see ccl-compile-lookup-integer used anywhere, FWIW.
I've CC'ed Handa-san, who might have some comments about this.

bug#36740: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)

[Eli Zaretskii]

> Date: Sat, 20 Jul 2019 16:15:52 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 36740@debbugs.gnu.org
> 
> I've CC'ed Handa-san, who might have some comments about this.

But the return email has no Kenichi Handa's address on the CC list.
What's going on? is debbugs removing addressees from the messages or
something? why?

bug#36740: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)

[Eli Zaretskii]

> Date: Sat, 20 Jul 2019 16:15:52 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 36740@debbugs.gnu.org
> 
> > From: Pip Cet <pipcet@gmail.com>
> > Date: Sat, 20 Jul 2019 12:29:57 +0000
> > 
> > This code in ccl.c
> > 
> >         eop = hash_lookup (h, make_fixnum (reg[RRR]), NULL);
> >         if (eop >= 0)
> >           {
> >             Lisp_Object opl;
> >             opl = HASH_VALUE (h, eop);
> >             if (! (IN_INT_RANGE (eop) && CHARACTERP (opl)))
> >               CCL_INVALID_CMD;
> >             reg[RRR] = charset_unicode;
> >             reg[rrr] = eop;
> >             reg[7] = 1; /* r7 true for success */
> >           }
> >         else
> >           reg[7] = 0;
> > 
> > seems wrong to me. We look up the hash value for reg[RRR], but then we
> > store the hash _index_ into reg[rrr], and throw away the actual value.
> 
> The comment for the op-code says:
> 
>   #define CCL_LookupIntConstTbl 0x13 /* Lookup multibyte character by
> 					integer key.  Afterwards R7 set
> 					to 1 if lookup succeeded.
> 					1:ExtendedCOMMNDRrrRRRXXXXXXXX
> 					2:ARGUMENT(Hash table ID) */
> 
> so there appears to be no significance to r7's value?

Actually, I think you are right.  In Emacs 22.1 we had this:

	    case CCL_LookupIntConstTbl:
	      op = XINT (ccl_prog[ic]); /* table */
	      ic++;
	      {
		struct Lisp_Hash_Table *h = GET_HASH_TABLE (op);

		op = hash_lookup (h, make_number (reg[RRR]), NULL);
		if (op >= 0)
		  {
		    Lisp_Object opl;
		    opl = HASH_VALUE (h, op);
		    if (!CHAR_VALID_P (XINT (opl), 0))
		      CCL_INVALID_CMD;
		    SPLIT_CHAR (XINT (opl), reg[RRR], i, j);
		    if (j != -1)
		      i = (i << 7) | j;
		    reg[rrr] = i;
		    reg[7] = 1; /* r7 true for success */
		  }
		else
		  reg[7] = 0;
	      }

So this was fixed at some point, but for some reason the fix didn't
make it into Emacs 23.

So yes, I think we should use the value of XINT(opl) here.

bug#36740: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)

[Pip Cet]

[-- Attachment #1: Type: text/plain, Size: 351 bytes --]

On Sat, Jul 20, 2019 at 1:51 PM Eli Zaretskii <eliz@gnu.org> wrote:
> So this was fixed at some point, but for some reason the fix didn't
> make it into Emacs 23.
>
> So yes, I think we should use the value of XINT(opl) here.

Patch attached. It'd be nice to have tests for this, of course, but
it'd be easier for someone who understands CCL to do...

[-- Attachment #2: 0001-Fix-return-value-for-CCL-opcode-lookup-integer.patch --]
[-- Type: application/x-patch, Size: 736 bytes --]

bug#36740: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)

[Andy Moreton]

On Sat 20 Jul 2019, Pip Cet wrote:

> On Sat, Jul 20, 2019 at 1:51 PM Eli Zaretskii <eliz@gnu.org> wrote:
>> So this was fixed at some point, but for some reason the fix didn't
>> make it into Emacs 23.
>>
>> So yes, I think we should use the value of XINT(opl) here.
>
> Patch attached. It'd be nice to have tests for this, of course, but
> it'd be easier for someone who understands CCL to do...

Are the tests in test/lisp/international/ccl-tests.el sufficient ? If
not, please etend them to cover this case.

    AndyM

bug#36740: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)

[Pip Cet]

[-- Attachment #1: Type: text/plain, Size: 664 bytes --]

On Sat, Jul 20, 2019 at 7:00 PM Andy Moreton <andrewjmoreton@gmail.com> wrote:
> > Patch attached. It'd be nice to have tests for this, of course, but
> > it'd be easier for someone who understands CCL to do...
>
> Are the tests in test/lisp/international/ccl-tests.el sufficient ? If
> not, please etend them to cover this case.

I'm trying, but it seems like the CCL code is somewhat broken:
`ccl-embed-symbol', as it is now, can never succeed, because it passes
a cons cell rather than a fixnum to (ultimately) logand.

The attached patch tries to fix that issue and adds a test. I've also
noticed I cannot use C-g to interrupt a CCL loop, is that intentional?

[-- Attachment #2: 0001-Fix-return-value-for-CCL-opcode-lookup-integer.patch --]
[-- Type: text/x-patch, Size: 2488 bytes --]

From dbad96e07aaa5038198047a0d2e4102c820f0b69 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sat, 20 Jul 2019 14:47:39 +0000
Subject: [PATCH] Fix return value for CCL opcode lookup-integer

* src/ccl.c (ccl_driver): Fix LookupIntConstTbl return value.
* test/lisp/international/ccl-tests.el (ccl-hash-table): Add test.
* lisp/international/ccl.el (ccl-fixnum): Don't pass non-numbers
to logand.
---
 lisp/international/ccl.el            |  4 +++-
 src/ccl.c                            |  2 +-
 test/lisp/international/ccl-tests.el | 14 ++++++++++++++
 3 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/lisp/international/ccl.el b/lisp/international/ccl.el
index 51626f5161..025cb50c06 100644
--- a/lisp/international/ccl.el
+++ b/lisp/international/ccl.el
@@ -190,7 +190,9 @@ ccl-current-ic
 ;; codeword to 28bits, and then sign extends the result to a fixnum.
 (defun ccl-fixnum (code)
   "Convert a CCL code word to a fixnum value."
-  (- (logxor (logand code #x0fffffff) #x08000000) #x08000000))
+  (if (integerp code)
+      (- (logxor (logand code #x0fffffff) #x08000000) #x08000000)
+    code))
 
 (defun ccl-embed-data (data &optional ic)
   "Embed integer DATA in `ccl-program-vector' at `ccl-current-ic' and
diff --git a/src/ccl.c b/src/ccl.c
index ff42c6f25f..da2559e0b1 100644
--- a/src/ccl.c
+++ b/src/ccl.c
@@ -1299,7 +1299,7 @@ #define EXCMD (field1 >> 6)
 		    if (! (IN_INT_RANGE (eop) && CHARACTERP (opl)))
 		      CCL_INVALID_CMD;
 		    reg[RRR] = charset_unicode;
-		    reg[rrr] = eop;
+		    reg[rrr] = XFIXNUM (opl);
 		    reg[7] = 1; /* r7 true for success */
 		  }
 		else
diff --git a/test/lisp/international/ccl-tests.el b/test/lisp/international/ccl-tests.el
index 69e3930d42..2de4d040c2 100644
--- a/test/lisp/international/ccl-tests.el
+++ b/test/lisp/international/ccl-tests.el
@@ -227,3 +227,17 @@ ccl-dump-midi
   (with-temp-buffer
     (ccl-dump prog-midi-code)
     (should (equal (buffer-string) prog-midi-dump))))
+
+(ert-deftest ccl-hash-table ()
+  (let ((sym (gensym))
+        (table (make-hash-table :test 'eq)))
+    (puthash 16 17 table)
+    (puthash 17 16 table)
+    (define-translation-hash-table sym table)
+    (let* ((prog `(2
+                   ((loop
+                     (lookup-integer ,sym r0 r1)))))
+           (compiled (ccl-compile prog))
+           (registers [17 0 0 0 0 0 0 0]))
+      (ccl-execute compiled registers)
+      (should (equal registers [2 16 0 0 0 0 0 1])))))
-- 
2.22.0

bug#36740: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)

[Andy Moreton]

On Sat 20 Jul 2019, Pip Cet wrote:

> On Sat, Jul 20, 2019 at 7:00 PM Andy Moreton <andrewjmoreton@gmail.com> wrote:
>> > Patch attached. It'd be nice to have tests for this, of course, but
>> > it'd be easier for someone who understands CCL to do...
>>
>> Are the tests in test/lisp/international/ccl-tests.el sufficient ? If
>> not, please etend them to cover this case.
>
> I'm trying, but it seems like the CCL code is somewhat broken:
> `ccl-embed-symbol', as it is now, can never succeed, because it passes
> a cons cell rather than a fixnum to (ultimately) logand.

Agreed - probably my fault when fixing this code up to prepare for
bignum support.

>  (defun ccl-fixnum (code)
>    "Convert a CCL code word to a fixnum value."
> -  (- (logxor (logand code #x0fffffff) #x08000000) #x08000000))
> +  (if (integerp code)
> +      (- (logxor (logand code #x0fffffff) #x08000000) #x08000000)
> +    code))

`ccl-fixnum' should only receive integer arguments, so perhaps this fix
should go in the call in `ccl-embed-data' instead:

    (aset ccl-program-vector ccl-current-ic
      (if (numberp data) (ccl-fixnum data) data))

Thanks,

    AndyM

bug#36740: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)

[Pip Cet]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

On Sat, Jul 20, 2019 at 10:49 PM Andy Moreton <andrewjmoreton@gmail.com> wrote:
> >  (defun ccl-fixnum (code)
> >    "Convert a CCL code word to a fixnum value."
> > -  (- (logxor (logand code #x0fffffff) #x08000000) #x08000000))
> > +  (if (integerp code)
> > +      (- (logxor (logand code #x0fffffff) #x08000000) #x08000000)
> > +    code))
>
> `ccl-fixnum' should only receive integer arguments, so perhaps this fix
> should go in the call in `ccl-embed-data' instead:
>
>     (aset ccl-program-vector ccl-current-ic
>       (if (numberp data) (ccl-fixnum data) data))

Agreed, revised patch attached.

Note that the test leaks entries in translation-hash-table-vector, but
I think it's cleaner to start with a new symbol each time we run the
test.

[-- Attachment #2: 0001-Fix-return-value-for-CCL-opcode-lookup-integer.patch --]
[-- Type: text/x-patch, Size: 3042 bytes --]

From 91383d6aa1900c426b0c876e2feef04269c2fd15 Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Sun, 21 Jul 2019 05:54:49 +0000
Subject: [PATCH] Fix return value for CCL opcode lookup-integer

* src/ccl.c (ccl_driver): Fix LookupIntConstTbl return value.
* test/lisp/international/ccl-tests.el (ccl-hash-table): Add test.
* lisp/international/ccl.el (ccl-embed-data): Don't pass non-numbers
to `ccl-fixnum'.
---
 lisp/international/ccl.el            |  8 ++++++--
 src/ccl.c                            |  2 +-
 test/lisp/international/ccl-tests.el | 14 ++++++++++++++
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/lisp/international/ccl.el b/lisp/international/ccl.el
index 51626f5161..db4f2462fe 100644
--- a/lisp/international/ccl.el
+++ b/lisp/international/ccl.el
@@ -196,7 +196,9 @@ ccl-embed-data
   "Embed integer DATA in `ccl-program-vector' at `ccl-current-ic' and
 increment it.  If IC is specified, embed DATA at IC."
   (if ic
-      (aset ccl-program-vector ic (ccl-fixnum data))
+      (aset ccl-program-vector ic (if (numberp data)
+                                      (ccl-fixnum data)
+                                    data))
     (let ((len (length ccl-program-vector)))
       (if (>= ccl-current-ic len)
 	  (let ((new (make-vector (* len 2) nil)))
@@ -204,7 +206,9 @@ ccl-embed-data
 	      (setq len (1- len))
 	      (aset new len (aref ccl-program-vector len)))
 	    (setq ccl-program-vector new))))
-    (aset ccl-program-vector ccl-current-ic (ccl-fixnum data))
+    (aset ccl-program-vector ccl-current-ic (if (numberp data)
+                                                (ccl-fixnum data)
+                                              data))
     (setq ccl-current-ic (1+ ccl-current-ic))))
 
 (defun ccl-embed-symbol (symbol prop)
diff --git a/src/ccl.c b/src/ccl.c
index ff42c6f25f..da2559e0b1 100644
--- a/src/ccl.c
+++ b/src/ccl.c
@@ -1299,7 +1299,7 @@ #define EXCMD (field1 >> 6)
 		    if (! (IN_INT_RANGE (eop) && CHARACTERP (opl)))
 		      CCL_INVALID_CMD;
 		    reg[RRR] = charset_unicode;
-		    reg[rrr] = eop;
+		    reg[rrr] = XFIXNUM (opl);
 		    reg[7] = 1; /* r7 true for success */
 		  }
 		else
diff --git a/test/lisp/international/ccl-tests.el b/test/lisp/international/ccl-tests.el
index 69e3930d42..2de4d040c2 100644
--- a/test/lisp/international/ccl-tests.el
+++ b/test/lisp/international/ccl-tests.el
@@ -227,3 +227,17 @@ ccl-dump-midi
   (with-temp-buffer
     (ccl-dump prog-midi-code)
     (should (equal (buffer-string) prog-midi-dump))))
+
+(ert-deftest ccl-hash-table ()
+  (let ((sym (gensym))
+        (table (make-hash-table :test 'eq)))
+    (puthash 16 17 table)
+    (puthash 17 16 table)
+    (define-translation-hash-table sym table)
+    (let* ((prog `(2
+                   ((loop
+                     (lookup-integer ,sym r0 r1)))))
+           (compiled (ccl-compile prog))
+           (registers [17 0 0 0 0 0 0 0]))
+      (ccl-execute compiled registers)
+      (should (equal registers [2 16 0 0 0 0 0 1])))))
-- 
2.22.0

bug#36740: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)

[Noam Postavsky]

Pip Cet <pipcet@gmail.com> writes:

> Note that the test leaks entries in translation-hash-table-vector, but
> I think it's cleaner to start with a new symbol each time we run the
> test.

I don't really see the point in using a fresh symbol here; but if you do
insist on that, let-binding translation-hash-table-vector should stop
the leak, right?

> +(ert-deftest ccl-hash-table ()
> +  (let ((sym (gensym))
> +        (table (make-hash-table :test 'eq)))
> +    (puthash 16 17 table)
> +    (puthash 17 16 table)
> +    (define-translation-hash-table sym table)
> +    (let* ((prog `(2
> +                   ((loop
> +                     (lookup-integer ,sym r0 r1)))))
> +           (compiled (ccl-compile prog))
> +           (registers [17 0 0 0 0 0 0 0]))
> +      (ccl-execute compiled registers)
> +      (should (equal registers [2 16 0 0 0 0 0 1])))))

bug#36740: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)

[Lars Ingebrigtsen]

Pip Cet <pipcet@gmail.com> writes:

> Agreed, revised patch attached.

Looks like this patch was never applied, but everybody seemed to be in
favour of it, so I applied it now.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no