bug#26058: utf16->string and utf32->string don't conform to R6RS

[Taylan Ulrich Bayırlı/Kammer]

See the R6RS Libraries document page 10. The differences:

- R6RS supports reading a BOM.

- R6RS mandates an endianness argument to specify the behavior at the
absence of a BOM.

- R6RS allows an optional third argument 'endianness-mandatory' to
explicitly ignore any possible BOM.

Here's a quick patch on top of master. I didn't test it thoroughly...


===File
/home/taylan/src/guile/guile-master/0001-Fix-R6RS-utf16-string-and-utf32-string.patch===
From f51cd1d4884caafb1ed0072cd77c0e3145f34576 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Taylan=20Ulrich=20Bay=C4=B1rl=C4=B1/Kammer?=
<***@gmail.com>
Date: Fri, 10 Mar 2017 22:36:55 +0100
Subject: [PATCH] Fix R6RS utf16->string and utf32->string.

* module/rnrs/bytevectors.scm (read-bom16, read-bom32): New procedures.
(r6rs-utf16->string, r6rs-utf32->string): Ditto.
---
module/rnrs/bytevectors.scm | 52 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/module/rnrs/bytevectors.scm b/module/rnrs/bytevectors.scm
index 9744359f0..997a8c9cb 100644
--- a/module/rnrs/bytevectors.scm
+++ b/module/rnrs/bytevectors.scm
@@ -69,7 +69,9 @@
bytevector-ieee-double-native-set!

string->utf8 string->utf16 string->utf32
- utf8->string utf16->string utf32->string))
+ utf8->string
+ (r6rs-utf16->string . utf16->string)
+ (r6rs-utf32->string . utf32->string)))


(load-extension (string-append "libguile-" (effective-version))
@@ -80,4 +82,52 @@
`(quote ,sym)
(error "unsupported endianness" sym)))

+(define (read-bom16 bv)
+ (let ((c0 (bytevector-u8-ref bv 0))
+ (c1 (bytevector-u8-ref bv 1)))
+ (cond
+ ((and (= c0 #xFE) (= c1 #xFF))
+ 'big)
+ ((and (= c0 #xFF) (= c1 #xFE))
+ 'little)
+ (else
+ #f))))
+
+(define r6rs-utf16->string
+ (case-lambda
+ ((bv default-endianness)
+ (let ((bom-endianness (read-bom16 bv)))
+ (if (not bom-endianness)
+ (utf16->string bv default-endianness)
+ (substring/shared (utf16->string bv bom-endianness) 1))))
+ ((bv endianness endianness-mandatory?)
+ (if endianness-mandatory?
+ (utf16->string bv endianness)
+ (r6rs-utf16->string bv endianness)))))
+
+(define (read-bom32 bv)
+ (let ((c0 (bytevector-u8-ref bv 0))
+ (c1 (bytevector-u8-ref bv 1))
+ (c2 (bytevector-u8-ref bv 2))
+ (c3 (bytevector-u8-ref bv 3)))
+ (cond
+ ((and (= c0 #x00) (= c1 #x00) (= c2 #xFE) (= c3 #xFF))
+ 'big)
+ ((and (= c0 #xFF) (= c1 #xFE) (= c2 #x00) (= c3 #x00))
+ 'little)
+ (else
+ #f))))
+
+(define r6rs-utf32->string
+ (case-lambda
+ ((bv default-endianness)
+ (let ((bom-endianness (read-bom32 bv)))
+ (if (not bom-endianness)
+ (utf32->string bv default-endianness)
+ (substring/shared (utf32->string bv bom-endianness) 1))))
+ ((bv endianness endianness-mandatory?)
+ (if endianness-mandatory?
+ (utf32->string bv endianness)
+ (r6rs-utf32->string bv endianness)))))
+
;;; bytevector.scm ends here

[Andy Wingo]

Post by Taylan Ulrich Bayırlı/Kammer
- R6RS supports reading a BOM.
- R6RS mandates an endianness argument to specify the behavior at the
absence of a BOM.
- R6RS allows an optional third argument 'endianness-mandatory' to
explicitly ignore any possible BOM.
Here's a quick patch on top of master. I didn't test it thoroughly...

Hi,

this is a tricky area that is not so amenable to quick patches :) Have
you looked into what Guile already does for byte-order marks? Can you
explain how the R6RS specification relates to this?

https://www.gnu.org/software/guile/manual/html_node/BOM-Handling.html

Andy

[Taylan Ulrich Bayırlı/Kammer]

Post by Andy Wingo
Hi,
this is a tricky area that is not so amenable to quick patches :) Have
you looked into what Guile already does for byte-order marks? Can you
explain how the R6RS specification relates to this?
https://www.gnu.org/software/guile/manual/html_node/BOM-Handling.html
Andy

Hmm, interesting. I noticed the utf{16,32}->string procedures ignoring
a BOM at the start of the given bytevector, but didn't look at it from a
ports perspective.

TL;DR of the below though: the R6RS semantics offer a strict enrichment
of the feature-set of the utfX->string procedures relative to the Guile
semantics, so at most we would end up with spurious features. (The
optional ability to handle any possible BOM at the start of the
bytevector, with a fall-back endianness in case none is found.)


That said, let's see...

If I do a textual read from a port, I already get a string and not a
bytevector, so the behavior of utfX->string operations is irrelevant.

If I do binary I/O, the following situations are possible:

1. I'm guaranteed to get any possible bytes that happen to form a valid
BOM at the start of the stream as-is in the returned bytevector; the
binary I/O interface doesn't see such bytes as anything special, as
it could simply be coincidence that the stream starts with such
bytes.

2. I'm guaranteed *not* to get bytes that form a BOM at the start of the
stream; instead they're consumed to set the port encoding for any
future text I/O.

3. The behavior is unspecified and either of the above may happen.

In the case of #1, it's probably good for utfX->string procedures to be
able to handle BOMs, but also allow explicitly ignoring any possible
BOM. The R6RS semantics cover this.

In the case of #2, the utfX->string procedures don't need to be able to
handle BOMs as far as we're talking about passing them bytevectors
returned by port I/O, but it also doesn't hurt if they optionally
support it. The R6RS semantics are fine here as well I think.

As for #3... first of all it's bad IMO; the behavior ought to be
specified. :-) But in any case, the additional features of the R6RS
semantics won't hurt.

WDYT? As far as I understand the page you linked, Guile currently
implements #3, which I think is unfortunate but can kinda understand
too. In any case, the additional R6RS features won't hurt, right?

Taylan

[Andy Wingo]

Post by Taylan Ulrich Bayırlı/Kammer
1. I'm guaranteed to get any possible bytes that happen to form a valid
BOM at the start of the stream as-is in the returned bytevector; the
binary I/O interface doesn't see such bytes as anything special, as
it could simply be coincidence that the stream starts with such
bytes.
2. I'm guaranteed *not* to get bytes that form a BOM at the start of the
stream; instead they're consumed to set the port encoding for any
future text I/O.
3. The behavior is unspecified and either of the above may happen.

(1). But I thought this bug was about using a bytevector as a source
and then doing textual I/O on it, no?

Andy

[Taylan Ulrich Bayırlı/Kammer]

Post by Andy Wingo

Post by Taylan Ulrich Bayırlı/Kammer
1. I'm guaranteed to get any possible bytes that happen to form a valid
BOM at the start of the stream as-is in the returned bytevector; the
binary I/O interface doesn't see such bytes as anything special, as
it could simply be coincidence that the stream starts with such
bytes.

(1). But I thought this bug was about using a bytevector as a source
and then doing textual I/O on it, no?

I have a feeling we're somehow talking past each other. :-) As far as
I'm concerned, the bug is just that the procedures don't conform to the
specification.

It would of course be good if the behavior of these procedures was
somehow in harmony with the behavior of I/O operations, but I don't see
any issues arising from adopting the R6RS behavior of the procedures
utf16->string and utf32->string. Do you?

Taylan

[Andy Wingo]

Post by Taylan Ulrich Bayırlı/Kammer

Post by Andy Wingo

Post by Taylan Ulrich Bayırlı/Kammer
1. I'm guaranteed to get any possible bytes that happen to form a valid
BOM at the start of the stream as-is in the returned bytevector; the
binary I/O interface doesn't see such bytes as anything special, as
it could simply be coincidence that the stream starts with such
bytes.

(1). But I thought this bug was about using a bytevector as a source
and then doing textual I/O on it, no?

I have a feeling we're somehow talking past each other. :-) As far as
I'm concerned, the bug is just that the procedures don't conform to the
specification.
It would of course be good if the behavior of these procedures was
somehow in harmony with the behavior of I/O operations, but I don't see
any issues arising from adopting the R6RS behavior of the procedures
utf16->string and utf32->string. Do you?

Adopting the behavior is more or less fine. If it can be done while
relying on the existing behavior, that is better than something ad-hoc
in a module.

Andy

[Taylan Ulrich Bayırlı/Kammer]

Post by Andy Wingo
Adopting the behavior is more or less fine. If it can be done while
relying on the existing behavior, that is better than something ad-hoc
in a module.

You mean somehow leveraging the existing BOM handling code of Guile
(found in the ports code) would be preferable to reimplementing it like
in this patch, correct?

In that light, I had this attempt:

(define r6rs-utf16->string
(case-lambda
((bv default-endianness)
(let* ((binary-port (open-bytevector-input-port bv))
(transcoder (make-transcoder (utf-16-codec)))
(utf16-port (transcoded-port binary-port transcoder)))
;; XXX how to set default-endianness for a port?
(get-string-all utf16-port)))
((bv endianness endianness-mandatory?)
(if endianness-mandatory?
(utf16->string bv endianness)
(r6rs-utf16->string bv endianness)))))

As commented in the first branch of the case-lambda, this does not yet
make use of the 'default-endianness' parameter to tell the port
transcoder (or whoever) what to do in case no BOM is found in the
stream.

From what I can tell, Guile is currently hardcoded to *transparently*
default to big-endian in ports.c, port_clear_stream_start_for_bom_read.

Is there a way to detect when Guile was unable to find a BOM? (In that
case one could set the endianness explicitly to the desired default.)

Or do you see another way to implement this?

Thanks for the feedback!
Taylan


P.S.: Huge congrats on the big release. :-)

[Mark H Weaver]

Hi Taylan,

Post by Taylan Ulrich Bayırlı/Kammer

Post by Andy Wingo
Adopting the behavior is more or less fine. If it can be done while
relying on the existing behavior, that is better than something ad-hoc
in a module.

In general, I agree with Andy's sentiment that it would be better to
avoid redundant BOM handling code, and moreover, I appreciate his
reluctance to apply a fix without careful consideration of our existing
BOM semantics.

However, as Taylan discovered, Guile does not provide a mechanism to
specify a default endianness of a UTF-16 or UTF-32 port in case a BOM is
not found. I see no straightforward way to implement these R6RS
interfaces using ports.

We could certainly add such a mechanism if needed, but I see another
problem with this approach: the expense of creating and later collecting
a bytevector port object would be a very heavy burden to place on these
otherwise fairly lightweight operations. Therefore, I would prefer to
avoid that implementation strategy for these operations.

Although BOM handling for ports is quite complex with many subtle points
to consider, detecting a BOM at the beginning of a bytevector is so
trivial that I personally have no objection to this tiny duplication of
logic.

Therefore, my preference would be to adopt code similar to that proposed

Post by Taylan Ulrich Bayırlı/Kammer
diff --git a/module/rnrs/bytevectors.scm b/module/rnrs/bytevectors.scm
index 9744359f0..997a8c9cb 100644
--- a/module/rnrs/bytevectors.scm
+++ b/module/rnrs/bytevectors.scm
@@ -69,7 +69,9 @@
bytevector-ieee-double-native-set!
string->utf8 string->utf16 string->utf32
- utf8->string utf16->string utf32->string))
+ utf8->string
+ (r6rs-utf16->string . utf16->string)
+ (r6rs-utf32->string . utf32->string)))
(load-extension (string-append "libguile-" (effective-version))
@@ -80,4 +82,52 @@
`(quote ,sym)
(error "unsupported endianness" sym)))
+(define (read-bom16 bv)
+ (let ((c0 (bytevector-u8-ref bv 0))
+ (c1 (bytevector-u8-ref bv 1)))
+ (cond
+ ((and (= c0 #xFE) (= c1 #xFF))
+ 'big)
+ ((and (= c0 #xFF) (= c1 #xFE))
+ 'little)
+ (else
+ #f))))

We should gracefully handle the case of an empty bytevector, returning
an empty string without error in that case.

Also, we should use a single 'bytevector-u16-ref' operation to check for
the BOM. Pick an arbitrary endianness for the operation (big-endian?),
and compare the resulting integer with both #xFEFF and #xFFFE. That
way, the code will be simpler and more efficient. Note that our VM has
dedicated instructions for these multi-byte bytevector accessors, and
there will be fewer comparison operations as well. Similarly for the
utf32 case.

What do you think?

Post by Taylan Ulrich Bayırlı/Kammer
+(define r6rs-utf16->string
+ (case-lambda
+ ((bv default-endianness)
+ (let ((bom-endianness (read-bom16 bv)))
+ (if (not bom-endianness)
+ (utf16->string bv default-endianness)
+ (substring/shared (utf16->string bv bom-endianness) 1))))

Better to use plain 'substring' here, I think. The machinery of shared
substrings is more expensive, and unnecessary in this case.

Otherwise, it looks good to me. Would you like to propose a revised
patch?

Andy, what do you think?

Mark