Re: Partial fix for libgcj/9802

[Mark Wielaard <mark at klomp dot org>]

Hi,
(I noticed that your email did not arrive on the mailinglist somehow.)

Note that the reason for the minimal 5 line patch were that we are very
close to a release. I hoped to solve the issue with UTF-8 surrogates
without impacting any other encoding issues. Otherwise I am afraid that
"the powers that be" will not accept the change for 3.3.

That said, I want to thank you very much for following up on this since
the hacker in me really want to see perfect code.

On Sun, 2003-02-23 at 01:09, James Clark wrote:
> Instead of moving bytes_todo from Output_UTF8 into UnicodeToBytes, I 
> would suggest adding a public method to UnicodeToBytes:
> 
>    public boolean havePendingBytes() {
>      return true;
>    }

Better to default to false to minimize impact to the other decoders.

> and then override this in Output_UTF8
> 
>    public boolean havePendingBytes() {
>      return bytes_todo > 0;
>    }
> 
> The reason is to give more flexibility to other converters.  They may 
> not work by having a count of pending bytes.  For example, SJIS has at 
> most one pending byte, so it has a field that is either -1 or the value 
> of that byte.  Thus in Output_SJIS you would add:
> 
>    public boolean havePendingBytes() {
>      return pending >= 0;
>    }
> 
> Similarly, in Output_EUCJIS you would add:
> 
>    public boolean havePendingBytes() {
>      return pending1 >= 0;
>    }
> 
> Not sure about Output_iconv.

It should be simple enough since my iconv manual says:
4. The output buffer has no more room for the next converted character.
   In this case it sets errno to E2BIG and returns (size_t)(-1).

But the natIconv.cc source code says:

  // Note that we can't check `errno' because
  // glibc 2.1.3 doesn't set it correctly.  We could check it if we
  // really needed to, but we'd have to disable support for 2.1.3.

Don't know how important supporting 2.1.3 is at this time. (Tom, I
assume you wrote this comment?)

But for now we can just always return false so the behaviour is the same
as before.

> Also I think there's scope for improvement in String::getBytes.  With 
> your change we have
> [...]
> Now in the case where todo == 0 && converter->bytes_todo > 0, 
> converter->write will return 0, thus causing buffer to be gratuitously 
> reallocated.  How about this instead?
> 
>    while (todo > 0  || converter->bytes_todo > 0)
>      {
>        converter->setOutput(buffer, bufpos);
>        int converted = converter->write(this, offset, todo, NULL);
>        if (converted == 0 && bufpos == converter->count)
> 	{
> 	  buflen *= 2;
> 	  jbyteArray newbuffer = JvNewByteArray(buflen);
> 	  memcpy (elements (newbuffer), elements (buffer), bufpos);
> 	  buffer = newbuffer;
> 	}
>        else
>          bufpos = converter->count
>        offset += converted;
>        todo -= converted;
>      }

Looks good, but I would use your newly introduced hasPendingBytes()
which seems a little bit more robust.

How does the following look:

2002-02-22  Mark Wielaard  <mark at klomp dot org>
            James Clark  <jjc at jclark dot com>

        Fix for PR libgcj/8738:
        * gnu/gcj/convert/UnicodeToBytes.java (havePendingByes): New method.
        * gnu/gcj/convert/Output_SJIS.java (havePendingByes): Likewise.
        * gnu/gcj/convert/Output_EUCJIS.java (havePendingByes): Likewise.
        * gnu/gcj/convert/Output_UTF8.java (havePendingByes): Likewise.
        (write): Always decrease avail when count is increased.
        * java/lang/natString.cc (getByes): Check converter havePendingBytes().

It passes the new surrogate Mauve tests (and introduces no regressions).

Cheers,

Mark

Index: java/lang/natString.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/natString.cc,v
retrieving revision 1.27
diff -u -r1.27 natString.cc
--- java/lang/natString.cc	13 Jun 2002 18:16:26 -0000	1.27
+++ java/lang/natString.cc	23 Feb 2003 22:28:04 -0000
@@ -1,6 +1,6 @@
 // natString.cc - Implementation of java.lang.String native methods.
 
-/* Copyright (C) 1998, 1999, 2000, 2001, 2002  Free Software Foundation
+/* Copyright (C) 1998, 1999, 2000, 2001, 2002, 2003  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -586,12 +586,12 @@
   jint offset = 0;
   gnu::gcj::convert::UnicodeToBytes *converter
     = gnu::gcj::convert::UnicodeToBytes::getEncoder(enc);
-  while (todo > 0)
+  while (todo > 0 || converter->havePendingBytes())
     {
       converter->setOutput(buffer, bufpos);
       int converted = converter->write(this, offset, todo, NULL);
       bufpos = converter->count;
-      if (converted == 0)
+      if (converted == 0 && converter->havePendingBytes())
 	{
 	  buflen *= 2;
 	  jbyteArray newbuffer = JvNewByteArray(buflen);
Index: gnu/gcj/convert/Output_EUCJIS.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/gnu/gcj/convert/Output_EUCJIS.java,v
retrieving revision 1.3
diff -u -r1.3 Output_EUCJIS.java
--- gnu/gcj/convert/Output_EUCJIS.java	7 Mar 2000 19:55:24 -0000	1.3
+++ gnu/gcj/convert/Output_EUCJIS.java	23 Feb 2003 22:28:04 -0000
@@ -1,4 +1,4 @@
-/* Copyright (C) 1999  Free Software Foundation
+/* Copyright (C) 1999, 2003  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -21,6 +21,11 @@
   public native int write (char[] inbuffer, int inpos, int inlength);
 
   public native int write (String str, int inpos, int inlength, char[] work);
+
+  public boolean havePendingBytes()
+  {
+    return pending1 >= 0;
+  }
 
   int pending1 = -1;
   int pending2;
Index: gnu/gcj/convert/Output_SJIS.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/gnu/gcj/convert/Output_SJIS.java,v
retrieving revision 1.3
diff -u -r1.3 Output_SJIS.java
--- gnu/gcj/convert/Output_SJIS.java	7 Mar 2000 19:55:24 -0000	1.3
+++ gnu/gcj/convert/Output_SJIS.java	23 Feb 2003 22:28:05 -0000
@@ -1,4 +1,4 @@
-/* Copyright (C) 1999  Free Software Foundation
+/* Copyright (C) 1999, 2003  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -21,6 +21,11 @@
   public native int write (char[] inbuffer, int inpos, int inlength);
 
   public native int write (String str, int inpos, int inlength, char[] work);
+
+  public boolean havePendingBytes()
+  {
+    return pending >= 0;
+  }
 
   int pending = -1;
 }
Index: gnu/gcj/convert/Output_UTF8.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/gnu/gcj/convert/Output_UTF8.java,v
retrieving revision 1.6
diff -u -r1.6 Output_UTF8.java
--- gnu/gcj/convert/Output_UTF8.java	8 Aug 2000 17:35:32 -0000	1.6
+++ gnu/gcj/convert/Output_UTF8.java	23 Feb 2003 22:28:05 -0000
@@ -1,4 +1,4 @@
-/* Copyright (C) 1999, 2000  Free Software Foundation
+/* Copyright (C) 1999, 2000, 2003  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -104,6 +104,7 @@
 	      {
 		value = (hi_part - 0xD800) * 0x400 + (ch - 0xDC00) + 0x10000;
 		buf[count++] = (byte) (0xF0 | (value >> 18));
+		avail--;
 		bytes_todo = 3;
 		hi_part = 0;
 	      }
@@ -118,4 +119,10 @@
       }
     return inpos - start_pos;
   }
+
+  public boolean havePendingBytes()
+  {
+    return bytes_todo > 0;
+  }
+
 }
Index: gnu/gcj/convert/UnicodeToBytes.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/gnu/gcj/convert/UnicodeToBytes.java,v
retrieving revision 1.9
diff -u -r1.9 UnicodeToBytes.java
--- gnu/gcj/convert/UnicodeToBytes.java	18 Aug 2001 03:56:01 -0000	1.9
+++ gnu/gcj/convert/UnicodeToBytes.java	23 Feb 2003 22:28:05 -0000
@@ -1,4 +1,4 @@
-/* Copyright (C) 1999, 2000, 2001  Free Software Foundation
+/* Copyright (C) 1999, 2000, 2001, 2003  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -144,6 +144,17 @@
     int srcEnd = inpos + (inlength > work.length ? work.length : inlength);
     str.getChars(inpos, srcEnd, work, 0);
     return write(work, 0, srcEnd - inpos);
+  }
+
+  /**
+   * Returns true when the converter has consumed some bytes that are
+   * not yet converted to characters because further continuation
+   * bytes are needed.  Defaults to false, should be overridden by
+   * decoders that internally store some bytes.
+   */
+  public boolean havePendingBytes()
+  {
+    return false;
   }
 
   /** Indicate that the converter is resuable.