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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 36740 in the body.
You can then email your comments to 36740 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.50; apparently buggy code in ccl.c (lookup-integer-constant)
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 bug appears to be present in:

commit d325055a00e658a38c1721fcc63ed1775dd8ccb3
Author: Dave Love <fx <at> 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.

Message #8 received at 36740 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36740 <at> debbugs.gnu.org, Kenichi Handa <handa <at> gnu.org>
Subject: Re: bug#36740: 27.0.50;
 apparently buggy code in ccl.c (lookup-integer-constant)
Date: Sat, 20 Jul 2019 16:15:52 +0300

> From: Pip Cet <pipcet <at> 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 <at> 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.

Message #11 received at 36740 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: 36740 <at> debbugs.gnu.org
Subject: Re: bug#36740: 27.0.50;
 apparently buggy code in ccl.c (lookup-integer-constant)
Date: Sat, 20 Jul 2019 16:23:15 +0300

> Date: Sat, 20 Jul 2019 16:15:52 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: 36740 <at> 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?

Message #14 received at 36740 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: pipcet <at> gmail.com, Kenichi Handa <handa <at> gnu.org>
Cc: 36740 <at> debbugs.gnu.org
Subject: Re: bug#36740: 27.0.50;
 apparently buggy code in ccl.c (lookup-integer-constant)
Date: Sat, 20 Jul 2019 16:51:38 +0300

> Date: Sat, 20 Jul 2019 16:15:52 +0300
> From: Eli Zaretskii <eliz <at> gnu.org>
> Cc: 36740 <at> debbugs.gnu.org
> 
> > From: Pip Cet <pipcet <at> 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.

Message #17 received at 36740 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Kenichi Handa <handa <at> gnu.org>, 36740 <at> debbugs.gnu.org
Subject: Re: bug#36740: 27.0.50;
 apparently buggy code in ccl.c (lookup-integer-constant)
Date: Sat, 20 Jul 2019 14:58:29 +0000

[Message part 1 (text/plain, inline)]

On Sat, Jul 20, 2019 at 1:51 PM Eli Zaretskii <eliz <at> 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...

[0001-Fix-return-value-for-CCL-opcode-lookup-integer.patch (application/x-patch, attachment)]

Message #20 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Andy Moreton <andrewjmoreton <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#36740: 27.0.50;
 apparently buggy code in ccl.c (lookup-integer-constant)
Date: Sat, 20 Jul 2019 19:59:35 +0100

On Sat 20 Jul 2019, Pip Cet wrote:

> On Sat, Jul 20, 2019 at 1:51 PM Eli Zaretskii <eliz <at> 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

Message #23 received at 36740 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> gmail.com>
To: Andy Moreton <andrewjmoreton <at> gmail.com>
Cc: 36740 <at> debbugs.gnu.org
Subject: Re: bug#36740: 27.0.50;
 apparently buggy code in ccl.c (lookup-integer-constant)
Date: Sat, 20 Jul 2019 20:18:21 +0000

[Message part 1 (text/plain, inline)]

On Sat, Jul 20, 2019 at 7:00 PM Andy Moreton <andrewjmoreton <at> 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?

[0001-Fix-return-value-for-CCL-opcode-lookup-integer.patch (text/x-patch, attachment)]

Message #26 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Andy Moreton <andrewjmoreton <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: bug#36740: 27.0.50;
 apparently buggy code in ccl.c (lookup-integer-constant)
Date: Sat, 20 Jul 2019 23:48:30 +0100

On Sat 20 Jul 2019, Pip Cet wrote:

> On Sat, Jul 20, 2019 at 7:00 PM Andy Moreton <andrewjmoreton <at> 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

Message #29 received at 36740 <at> debbugs.gnu.org (full text, mbox):

From: Pip Cet <pipcet <at> gmail.com>
To: Andy Moreton <andrewjmoreton <at> gmail.com>
Cc: 36740 <at> debbugs.gnu.org
Subject: Re: bug#36740: 27.0.50;
 apparently buggy code in ccl.c (lookup-integer-constant)
Date: Sun, 21 Jul 2019 06:01:10 +0000

[Message part 1 (text/plain, inline)]

On Sat, Jul 20, 2019 at 10:49 PM Andy Moreton <andrewjmoreton <at> 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.

[0001-Fix-return-value-for-CCL-opcode-lookup-integer.patch (text/x-patch, attachment)]

Message #32 received at 36740 <at> debbugs.gnu.org (full text, mbox):

From: Noam Postavsky <npostavs <at> gmail.com>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36740 <at> debbugs.gnu.org, Andy Moreton <andrewjmoreton <at> gmail.com>
Subject: Re: bug#36740: 27.0.50;
 apparently buggy code in ccl.c (lookup-integer-constant)
Date: Thu, 01 Aug 2019 13:14:43 -0400

Pip Cet <pipcet <at> 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])))))

Message #35 received at 36740 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Pip Cet <pipcet <at> gmail.com>
Cc: 36740 <at> debbugs.gnu.org, Andy Moreton <andrewjmoreton <at> gmail.com>
Subject: Re: bug#36740: 27.0.50; apparently buggy code in ccl.c
 (lookup-integer-constant)
Date: Fri, 21 Aug 2020 14:48:26 +0200

Pip Cet <pipcet <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.