Your INTERMEDIATE_ENCODING patch for Solaris

Your INTERMEDIATE_ENCODING patch for Solaris

[Kazu Hirata]

Hi Tom,

We've been using a version of your patch posted at:

  http://sourceware.org/ml/gdb-patches/2009-07/msg00434.html

except that we use

  #ifdef __sun__

rather than

  #ifdef __STDC_ISO_10646__

to special case for Solaris.

As Andrew replied to your message above, __STDC_ISO_10646__ is not
defined on Solaris.  Could we use __sun__ instead?

Or if that's not enough, we could try something like:

  #if defined(__sun__) || defined(__STDC_ISO_10646__)

Thoughts?

Thanks in advance,

Kazu Hirata

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Kazu" == Kazu Hirata <kazu@codesourcery.com> writes:

Kazu> We've been using a version of your patch posted at:
Kazu>   http://sourceware.org/ml/gdb-patches/2009-07/msg00434.html
Kazu> except that we use
Kazu>   #ifdef __sun__
Kazu> rather than
Kazu>   #ifdef __STDC_ISO_10646__
Kazu> to special case for Solaris.

Kazu> As Andrew replied to your message above, __STDC_ISO_10646__ is not
Kazu> defined on Solaris.  Could we use __sun__ instead?

I am not a Solaris expert, but some digging through the online
OpenSolaris libc sources a few months ago convinced me that the Solaris
wchar_t is in fact not UCS-4.

If that is true, then this patch would be incorrect.

My recollection is that if your locale's encoding was a stateful one,
then the Solaris wchar_t held the state in the high bits and the
character in the low bits; so I suppose this would be one way to try to
test to verify this hypothesis.  A better way would be to check out the
libc and read through it oneself.

This particular problem has come up multiple times.  I am not completely
sure what to do about it, but my current inclination is to change gdb so
that most hosts fall back to PHONY_ICONV, with exceptions for libiconv
and for Linux.  This will at least have gdb default to working, and is
also attractive because I have some hope of actually testing it.

Tom

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Daniel Jacobowitz]

On Thu, Aug 05, 2010 at 12:40:35PM -0600, Tom Tromey wrote:
> This particular problem has come up multiple times.  I am not completely
> sure what to do about it, but my current inclination is to change gdb so
> that most hosts fall back to PHONY_ICONV, with exceptions for libiconv
> and for Linux.  This will at least have gdb default to working, and is
> also attractive because I have some hope of actually testing it.

Sounds good to me.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Kazu> As Andrew replied to your message above, __STDC_ISO_10646__ is not
Kazu> defined on Solaris.  Could we use __sun__ instead?

Tom> I am not a Solaris expert, but some digging through the online
Tom> OpenSolaris libc sources a few months ago convinced me that the Solaris
Tom> wchar_t is in fact not UCS-4.

Tom> If that is true, then this patch would be incorrect.

While looking at this more today, I found:

    http://defect.opensolaris.org/bz/show_bug.cgi?id=11076

Some of the comments make it clear that wchar_t != UCS-4 for Solaris.

Tom> This particular problem has come up multiple times.  I am not completely
Tom> sure what to do about it, but my current inclination is to change gdb so
Tom> that most hosts fall back to PHONY_ICONV, with exceptions for libiconv
Tom> and for Linux.  This will at least have gdb default to working, and is
Tom> also attractive because I have some hope of actually testing it.

Daniel> Sounds good to me.

Here is what I am testing.  If someone can try it out on Solaris, I
would appreciate that.

Tom

b/gdb/ChangeLog:
2010-08-17  Tom Tromey  <tromey@redhat.com>

	* gdb_wchar.h: Check HAVE_LIBICONV, __STDC_ISO_10646__.  Change
	how INTERMEDIATE_ENCODING is defined.
	* configure, config.in: Rebuild.
	* acinclude.m4 (AM_ICONV): Define HAVE_LIBICONV.

diff --git a/gdb/acinclude.m4 b/gdb/acinclude.m4
index 1942ef4..082a9bc 100644
--- a/gdb/acinclude.m4
+++ b/gdb/acinclude.m4
@@ -256,6 +256,7 @@ AC_DEFUN([AM_ICONV],
   LIBICONV=
   if test "$am_cv_lib_iconv" = yes; then
     LIBICONV="-liconv"
+    AC_DEFINE(HAVE_LIBICONV, 1, [Define if using libiconv.])
   else
     LIBICONV_LIBDIR=
     LIBICONV_INCLUDE=
diff --git a/gdb/config.in b/gdb/config.in
index 7659181..35272a2 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -214,6 +214,9 @@
 /* Define if you have the expat library. */
 #undef HAVE_LIBEXPAT
 
+/* Define if using libiconv. */
+#undef HAVE_LIBICONV
+
 /* Define to 1 if you have the `libiconvlist' function. */
 #undef HAVE_LIBICONVLIST
 
diff --git a/gdb/configure b/gdb/configure
index 485c904..852c5c1 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -9450,6 +9450,9 @@ $as_echo "$am_cv_func_iconv" >&6; }
   LIBICONV=
   if test "$am_cv_lib_iconv" = yes; then
     LIBICONV="-liconv"
+
+$as_echo "#define HAVE_LIBICONV 1" >>confdefs.h
+
   else
     LIBICONV_LIBDIR=
     LIBICONV_INCLUDE=
diff --git a/gdb/gdb_wchar.h b/gdb/gdb_wchar.h
index fca3fe4..1ff7a1c 100644
--- a/gdb/gdb_wchar.h
+++ b/gdb/gdb_wchar.h
@@ -23,7 +23,11 @@
    
    Capable systems have the full suite: wchar_t support and iconv
    (perhaps via GNU libiconv).  On these machines, full functionality
-   is available.
+   is available.  Note that full functionality is dependent on us
+   being able to convert from an arbitrary encoding to wchar_t.  In
+   practice this means we look for __STDC_ISO_10646__ (where we know
+   the name of the wchar_t encoding) or GNU libiconv, where we can use
+   "wchar_t".
    
    DJGPP is known to have libiconv but not wchar_t support.  On
    systems like this, we use the narrow character functions.  The full
@@ -35,8 +39,6 @@
    wrappers for the wchar_t functionality we use.  */
 
 
-#define INTERMEDIATE_ENCODING "wchar_t"
-
 #if defined (HAVE_ICONV)
 #include <iconv.h>
 #else
@@ -45,9 +47,11 @@
 #define PHONY_ICONV
 #endif
 
-/* We use "btowc" as a sentinel to detect functioning wchar_t
-   support.  */
-#if defined (HAVE_ICONV) && defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC)
+/* We use "btowc" as a sentinel to detect functioning wchar_t support.
+   We check for either __STDC_ISO_10646__ or libiconv in order to
+   ensure we can convert to and from wchar_t.  */
+#if defined (HAVE_ICONV) && defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC) \
+  && (defined (__STDC_ISO_10646__) || defined (HAVE_LIBICONV))
 
 #include <wchar.h>
 #include <wctype.h>
@@ -63,6 +67,25 @@ typedef wint_t gdb_wint_t;
 
 #define LCST(X) L ## X
 
+/* If __STDC_ISO_10646__ is defined, then the host wchar_t is UCS-4.
+   We exploit this fact in the hope that there are hosts that define
+   this but which do not support "wchar_t" as an encoding argument to
+   iconv_open.  We put the endianness into the encoding name to avoid
+   hosts that emit a BOM when the unadorned name is used.  */
+#if defined (__STDC_ISO_10646__)
+#if WORDS_BIGENDIAN
+#define INTERMEDIATE_ENCODING "UCS-4BE"
+#else
+#define INTERMEDIATE_ENCODING "UCS-4LE"
+#endif
+#elif defined (HAVE_LIBICONV)
+#define INTERMEDIATE_ENCODING "wchar_t"
+#else
+/* This shouldn't happen, because the earlier #if should have filtered
+   out this case.  */
+#error "Neither __STDC_ISO_10646__ nor HAVE_LIBICONV defined"
+#endif
+
 #else
 
 typedef char gdb_wchar_t;

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Joel Brobecker]

> Here is what I am testing.  If someone can try it out on Solaris, I
> would appreciate that.
> 
> Tom
> 
> b/gdb/ChangeLog:
> 2010-08-17  Tom Tromey  <tromey@redhat.com>
> 
> 	* gdb_wchar.h: Check HAVE_LIBICONV, __STDC_ISO_10646__.  Change
> 	how INTERMEDIATE_ENCODING is defined.
> 	* configure, config.in: Rebuild.
> 	* acinclude.m4 (AM_ICONV): Define HAVE_LIBICONV.

What type of testing do you need? (I assume you need GDB to be built
without GNU libiconv?) Run the testsuite before and after patch? Or
something a little more specialized?

-- 
Joel

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> What type of testing do you need? (I assume you need GDB to be built
Joel> without GNU libiconv?) Run the testsuite before and after patch? Or
Joel> something a little more specialized?

I think that without this patch, a Solaris build (without libiconv) is
just pretty broken.  I think even `print "string"' should fail.

If not, it should be sufficient to look at the charset.exp results.

Tom

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Joel Brobecker]

> I think that without this patch, a Solaris build (without libiconv) is
> just pretty broken.  I think even `print "string"' should fail.

I'm a little confused at the moment. `print "string"' works and I get
only PASSes with one UNTESTED due to the fact that we only have 2
charsets available. Would it be because I have not configure GDB
in a way that reproduces the problem?

And then I did something that will bring the wrath of the NY sysadmin
team because I ran the entire testsuite, which I forgot brings the
machine down. So I cannot say more about the general results :-(.

-- 
Joel

RE: Your INTERMEDIATE_ENCODING patch for Solaris

[Pierre Muller]

  I can confirm that on a 32-bit i386 OpenSolaris 2.11,
the current CVS tree has the ICONV problem, which is solved by this patch.
Thus I fully support your patch, with a small modification required
for PHONY_ICONV, see below.

  The easiest way to see the difference is
(gdbcvs is the executable generated by current CVS HEAD,
gdb is the patched executable).

With:
./gdbcvs ./gdb
.....
(top-gdb) p version
$1 = <error reading variable>
While with:
./gdb ./gdb
.....
(top-gdb) p version
$1 = "7.2.50.20100818-cvs"

  I do have other problems with the x86_64 and sparc OpenSolaris versions
I tried to compile, but those are not directly related to the ICONV issue
(at least not only!).

  For the x86_64, I had two linking problems:
1) gcc claims that it doesn't know -rdynamic option
gcc --version returns this:
gcc (GCC) 3.4.3 (csl-sol210-3_4-20050802)

"gcc --print-prog-name=ld" returned "/usr/ccs/bin/ld"

and "`gcc --print-prog-name=ld` --version" returned:
ld: Software Generation Utilities - Solaris Link Editors: 5.11-1.1689

The compilation was done using the 5 lines long script:
#!/usr/bin/env bash
mkdir build64
cd build64
../src/configure --build=x86_64-pc-solaris2.11 --disable-gdbtk CFLAGS="-g -O0 -m64"
make all-gdb

2) gdb_curses.h was loading ncurses/ncurses.h header,
but the library was /lib/64/libcurses.so.1, not a ncurses library.

This led to unresolved symbols waddr_on/waddr_off (coming from
macro definitions  waddron and waddroff inside "ncurses/ncurses.h" header).
(I finally found the origin of that problem:
I installed libiconv  from BlastWave into /opt/csw directory,
but this directory was not searched for libraries..).

  Manually disabling the 
#define HAVE_NCURSES_NCURSES_H 1
line in config.h 
allowed to complete the compilation successfully.
  I don't know if this is just an installation problem
on my side or a more general issue.

  Here the generated debugger did not show the 
ICONV problem, as no iconv function was found.

  After application of your patch however, I get this:
gcc -g -O0 -m64   -I. -I../../src/gdb -I../../src/gdb/common -I../../src/gdb/con
fig -DLOCALEDIR="\"/usr/local/share/locale\"" -DHAVE_CONFIG_H -I../../src/gdb/..
/include/opcode -I../../src/gdb/../opcodes/.. -I../../src/gdb/../readline/.. -I.
./bfd -I../../src/gdb/../bfd -I../../src/gdb/../include -I../libdecnumber -I../.
./src/gdb/../libdecnumber  -I../../src/gdb/gnulib -Ignulib  -DMI_OUT=1 -DTUI=1
-I/usr/include -I/usr/include  -Wall -Wdeclaration-after-statement -Wpointer-ari
th -Wformat-nonliteral -Wno-unused -Wunused-value -Wunused-function -Wno-switch
-Wno-char-subscripts -Werror -c -o charset.o -MT charset.o -MMD -MP -MF .deps/ch
arset.Tpo ../../src/gdb/charset.c
../../src/gdb/charset.c: In function `make_wchar_iterator':
../../src/gdb/charset.c:585: error: `INTERMEDIATE_ENCODING' undeclared (first us
e in this function)
../../src/gdb/charset.c:585: error: (Each undeclared identifier is reported only
 once
../../src/gdb/charset.c:585: error: for each function it appears in.)
make: *** [charset.o] Error 1

  This is because there is an error in your patch:
if PHONY_ICONV is defined, INTERMEDIATE_ENCODING is not set at all
anymore.
  Removing the 
#ifndef PHONY_ICONV
around INTERMEDIATE_ENCODING definition
in the bottom of gdb_wchar.h file fixes that issue.

 I still needed to disable HAVE_NUCURSES_NCURSES_H to be able
to compile GDB.

 I will try to use CSW libiconv to see what happens then.


Pierre Muller
Pascal language support maintainer for GDB

RE: Your INTERMEDIATE_ENCODING patch for Solaris

[Pierre Muller]

Upon further investigation, I must confess that the OpenSolaris 
situation is a real mess:

  I get a working libiconv version for x86_64 solaris
using CSW libiconv version 0x10D,
libiconv_open with parameter 'wchar_t' and 'UTF-8' works.
but on my Sparc64 test machine, libiconv is
on /usr/local/lib and its version is 0x10B.
This later version fails on calls to libiconv_open
with 'wchar_t' and 'ASCII'.
  Then I discovered a difference:
  I has LANG environment variable set in the first (x86_64 machine)
to en_US.UTF-8
so I tried to use that environment variable on the Sparc machine
and, o wonder, gdb starts to work!
  So the Sparc iconv works if LANG is set to 'en_US.UTF-8',
but fails it LANG is not set.

  Coming back to the x86_64 machine,
I checked what happened if I removed the LANG environment variable...
in that case it is still working, libiconv_open (ver 0x10D) accepts
'ASCII' to 'wchar_t' conversion, while version 0x10B seems to fail...
At least that is where I am now, but it might not be the 
entire picture ...

  
  
  
Pierre Muller
Pascal language support maintainer for GDB

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Joel Brobecker]

IMO, investing too much energy into making GDB work with a "difficult"
(aka broken) libiconv is a waste of precious time. Perhaps it's time
for us to cut the losses and decide that building GDB requires GNU
libiconv.  I was first in line asking that GDB could be built without
GNU libiconv, but I can see from experience how much of a mess that is...

?

-- 
Joel

RE: Your INTERMEDIATE_ENCODING patch for Solaris

[Pierre Muller]

  How can we identity that the installed library is a GNU one or not?
I have no idea if the ones I tested are GNU or not.
  
Pierre

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : Wednesday, August 18, 2010 4:52 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: Your INTERMEDIATE_ENCODING patch for Solaris
> 
> 
> IMO, investing too much energy into making GDB work with a "difficult"
> (aka broken) libiconv is a waste of precious time. Perhaps it's time
> for us to cut the losses and decide that building GDB requires GNU
> libiconv.  I was first in line asking that GDB could be built without
> GNU libiconv, but I can see from experience how much of a mess that
> is...
> 
> ?
> 
> --
> Joel

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> IMO, investing too much energy into making GDB work with a "difficult"
Joel> (aka broken) libiconv is a waste of precious time. Perhaps it's time
Joel> for us to cut the losses and decide that building GDB requires GNU
Joel> libiconv.  I was first in line asking that GDB could be built without
Joel> GNU libiconv, but I can see from experience how much of a mess that is...

Joel> ?

I think we're ok.  The hard part is already done -- all the PHONY_ICONV
support code and typedefs and indirections.  This last bit is just
tinkering with the configury a little to get a decent result by default.

FWIW, all this current change means is that Solaris users will see
escape sequences more often than they might want to.

Tom

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Tom> I think that without this patch, a Solaris build (without libiconv) is
Tom> just pretty broken.  I think even `print "string"' should fail.

Joel> I'm a little confused at the moment. `print "string"' works and I get
Joel> only PASSes with one UNTESTED due to the fact that we only have 2
Joel> charsets available. Would it be because I have not configure GDB
Joel> in a way that reproduces the problem?

Did you build with libiconv?  libiconv fixes all the problems -- the
issue only shows up if you do a plain Solaris build.

Tom

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre> Upon further investigation, I must confess that the OpenSolaris 
Pierre> situation is a real mess:

Pierre>   I get a working libiconv version for x86_64 solaris
Pierre> using CSW libiconv version 0x10D,

From the version number, I guess this is GNU libiconv.
What happens if you don't build with libiconv, but rely instead on the
iconv in the Solaris libc?

Pierre> libiconv_open with parameter 'wchar_t' and 'UTF-8' works.
Pierre> but on my Sparc64 test machine, libiconv is
Pierre> on /usr/local/lib and its version is 0x10B.
Pierre> This later version fails on calls to libiconv_open
Pierre> with 'wchar_t' and 'ASCII'.

Ouch, but ok.  We can check _LIBICONV_VERSION >= 0x10D.
This actually simplifies the patch a little; I didn't know about
_LIBICONV_VERSION before.

Could you try this?  I also fixed the INTERMEDIATE_ENCODING problem you
pointed out.

Tom

2010-08-17  Tom Tromey  <tromey@redhat.com>

	* gdb_wchar.h: Check _LIBICONV_VERSION, __STDC_ISO_10646__.
	Change how INTERMEDIATE_ENCODING is defined.

diff --git a/gdb/gdb_wchar.h b/gdb/gdb_wchar.h
index fca3fe4..78f59a1 100644
--- a/gdb/gdb_wchar.h
+++ b/gdb/gdb_wchar.h
@@ -23,7 +23,11 @@
    
    Capable systems have the full suite: wchar_t support and iconv
    (perhaps via GNU libiconv).  On these machines, full functionality
-   is available.
+   is available.  Note that full functionality is dependent on us
+   being able to convert from an arbitrary encoding to wchar_t.  In
+   practice this means we look for __STDC_ISO_10646__ (where we know
+   the name of the wchar_t encoding) or GNU libiconv, where we can use
+   "wchar_t".
    
    DJGPP is known to have libiconv but not wchar_t support.  On
    systems like this, we use the narrow character functions.  The full
@@ -35,8 +39,6 @@
    wrappers for the wchar_t functionality we use.  */
 
 
-#define INTERMEDIATE_ENCODING "wchar_t"
-
 #if defined (HAVE_ICONV)
 #include <iconv.h>
 #else
@@ -45,9 +47,15 @@
 #define PHONY_ICONV
 #endif
 
-/* We use "btowc" as a sentinel to detect functioning wchar_t
-   support.  */
-#if defined (HAVE_ICONV) && defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC)
+/* We use "btowc" as a sentinel to detect functioning wchar_t support.
+   We check for either __STDC_ISO_10646__ or a new-enough libiconv in
+   order to ensure we can convert to and from wchar_t.  We choose
+   libiconv version 0x10D because it was reported that earlier
+   versions do not always accept "wchar_t" as an encoding argument on
+   Solaris.  */
+#if defined (HAVE_ICONV) && defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC) \
+  && (defined (__STDC_ISO_10646__) \
+      || defined (_LIBICONV_VERSION) && _LIBICONV_VERSION >= 0x10D)
 
 #include <wchar.h>
 #include <wctype.h>
@@ -63,6 +71,25 @@ typedef wint_t gdb_wint_t;
 
 #define LCST(X) L ## X
 
+/* If __STDC_ISO_10646__ is defined, then the host wchar_t is UCS-4.
+   We exploit this fact in the hope that there are hosts that define
+   this but which do not support "wchar_t" as an encoding argument to
+   iconv_open.  We put the endianness into the encoding name to avoid
+   hosts that emit a BOM when the unadorned name is used.  */
+#if defined (__STDC_ISO_10646__)
+#if WORDS_BIGENDIAN
+#define INTERMEDIATE_ENCODING "UCS-4BE"
+#else
+#define INTERMEDIATE_ENCODING "UCS-4LE"
+#endif
+#elif defined (_LIBICONV_VERSION) && _LIBICONV_VERSION >= 0x10D
+#define INTERMEDIATE_ENCODING "wchar_t"
+#else
+/* This shouldn't happen, because the earlier #if should have filtered
+   out this case.  */
+#error "Neither __STDC_ISO_10646__ nor _LIBICONV_VERSION defined"
+#endif
+
 #else
 
 typedef char gdb_wchar_t;
@@ -80,8 +107,9 @@ typedef int gdb_wint_t;
    narrow encoding as our intermediate encoding.  However, if we are
    also providing a phony iconv, we might as well just stick with
    "wchar_t".  */
-#ifndef PHONY_ICONV
-#undef INTERMEDIATE_ENCODING
+#ifdef PHONY_ICONV
+#define INTERMEDIATE_ENCODING "wchar_t"
+#else
 #define INTERMEDIATE_ENCODING host_charset ()
 #endif
 

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Joel Brobecker]

> Did you build with libiconv?  libiconv fixes all the problems -- the
> issue only shows up if you do a plain Solaris build.

I definitely tried building without libiconv. Looking at the difference
in behavior, with GNU libiconv, I get some 350 possible charsets, whereas
with the one I tested, I only have 2...

... Since Pierre managed to reproduce some of the problem and provide
additional info, I'll let him do the helping.

-- 
Joel

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> Did you build with libiconv?  libiconv fixes all the problems -- the
>> issue only shows up if you do a plain Solaris build.

Joel> I definitely tried building without libiconv. Looking at the difference
Joel> in behavior, with GNU libiconv, I get some 350 possible charsets, whereas
Joel> with the one I tested, I only have 2...

Strange.  I can't explain that.  Maybe it could be dependent on the
version of Solaris?

Tom

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Joel Brobecker]

> Strange.  I can't explain that.  Maybe it could be dependent on the
> version of Solaris?

It's very possible. I should have said that I was testing on 2.8,
if that helps at all.

-- 
Joel

RE: Your INTERMEDIATE_ENCODING patch for Solaris

[Pierre Muller]

   Hi Tom,

  I think your patch still has a problem:
if I have a system (like my i386 open solaris)
that only has the libc iconv functions, but no libiconv)
The libc headers do not set __STDC_ISO_10646__ 
nor defined _LIBICONV_VERSION macro.

  Thus we do not know how to handle those iconv functions.
I think that we should simply ignore them by defining PHONY_ICONV in such cases.
The problem is then that we already loaded libc iconv.h header
which then leads to an error of conflicting types for iconv definition.

  This probably means that the check that in done inside gdb_wchar.h should be
changed into a configuration macro, something like
  HAVE_USABLE_ICONV.

Pierre

PS: libc iconv on Solaris seems to work more or less,
but it doesn't like 'ASCII' charset (don't know why..)
but code in _initialize_charset does transform '646' into 'ASCII'
which is bad.
  If LANG is set to en_US.UTF-8 for instance,
the  'print version' works.
So changing the '646' -> 'ASCII' into a '646' -> 'UTF-8' 
would also improve things for solaris, but I have no idea
if this would also affect Non Solaris hosts.
  

PS2: what about being able to disable iconv at configure level with
--disable-iconv
that would skip iconv checks and thus lead to
PHONY_ICONV?

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>    Hi Tom,

Pierre>   I think your patch still has a problem:
Pierre> if I have a system (like my i386 open solaris)
Pierre> that only has the libc iconv functions, but no libiconv)
Pierre> The libc headers do not set __STDC_ISO_10646__ 
Pierre> nor defined _LIBICONV_VERSION macro.

Pierre>   Thus we do not know how to handle those iconv functions.

I don't really understand.  Does this show up as a compile-time error?
Or a runtime problem?  Could you give more details?

Pierre> I think that we should simply ignore them by defining
Pierre> PHONY_ICONV in such cases.

Yeah, we can do this.

This reduces functionality a lot, and since Solaris does have a mostly
working iconv it seems like we should try to use as much as we can.
But, Solaris seems to make this as difficult as possible.

Pierre> PS: libc iconv on Solaris seems to work more or less,
Pierre> but it doesn't like 'ASCII' charset (don't know why..)
Pierre> but code in _initialize_charset does transform '646' into 'ASCII'
Pierre> which is bad.

Oh, interesting.  The reports I have heard are that, on Solaris,
nl_langinfo(CODESET) returns "646", which is not a valid argument to
iconv_open.  I thought we knew that "ASCII" did work, but maybe it is
version dependent or something.

I think this is moot if we make the PHONY_ICONV change.

Pierre> PS2: what about being able to disable iconv at configure level with
Pierre> --disable-iconv
Pierre> that would skip iconv checks and thus lead to
Pierre> PHONY_ICONV?

I'd prefer to avoid more configure options unless absolutely necessary.
This area already has a lot of modes.

Here's a new version that should disable iconv on this platform, among
others.

Tom

b/gdb/ChangeLog:
2010-08-19  Tom Tromey  <tromey@redhat.com>

	* charset.c (iconv_open): New define.
	(iconv): Likewise.
	(iconv_close): Likewise.
	(phony_iconv_open): Add "phony_" prefix.
	(phony_iconv_close): Likewise.
	(phony_iconv): Likewise.
	* gdb_wchar.h: Check _LIBICONV_VERSION, __STDC_ISO_10646__.
	Change how INTERMEDIATE_ENCODING is defined.

diff --git a/gdb/charset.c b/gdb/charset.c
index 43b7fa8..3274219 100644
--- a/gdb/charset.c
+++ b/gdb/charset.c
@@ -90,8 +90,11 @@
 #undef iconv_t
 #define iconv_t int
 #undef iconv_open
+#define iconv_open phony_iconv_open
 #undef iconv
+#define iconv phony_iconv
 #undef iconv_close
+#define iconv_close phony_iconv_close
 
 #undef ICONV_CONST
 #define ICONV_CONST const
@@ -106,7 +109,7 @@
 #endif
 
 iconv_t
-iconv_open (const char *to, const char *from)
+phony_iconv_open (const char *to, const char *from)
 {
   /* We allow conversions from UTF-32BE, wchar_t, and the host charset.
      We allow conversions to wchar_t and the host charset.  */
@@ -122,14 +125,14 @@ iconv_open (const char *to, const char *from)
 }
 
 int
-iconv_close (iconv_t arg)
+phony_iconv_close (iconv_t arg)
 {
   return 0;
 }
 
 size_t
-iconv (iconv_t utf_flag, const char **inbuf, size_t *inbytesleft,
-       char **outbuf, size_t *outbytesleft)
+phony_iconv (iconv_t utf_flag, const char **inbuf, size_t *inbytesleft,
+	     char **outbuf, size_t *outbytesleft)
 {
   if (utf_flag)
     {
diff --git a/gdb/gdb_wchar.h b/gdb/gdb_wchar.h
index fca3fe4..7b0f958 100644
--- a/gdb/gdb_wchar.h
+++ b/gdb/gdb_wchar.h
@@ -23,20 +23,24 @@
    
    Capable systems have the full suite: wchar_t support and iconv
    (perhaps via GNU libiconv).  On these machines, full functionality
-   is available.
+   is available.  Note that full functionality is dependent on us
+   being able to convert from an arbitrary encoding to wchar_t.  In
+   practice this means we look for __STDC_ISO_10646__ (where we know
+   the name of the wchar_t encoding) or GNU libiconv, where we can use
+   "wchar_t".
    
    DJGPP is known to have libiconv but not wchar_t support.  On
    systems like this, we use the narrow character functions.  The full
    functionality is available to the user, but many characters (those
    outside the narrow range) will be displayed as escapes.
    
-   Finally, some systems do not have iconv.  Here we provide a phony
-   iconv which only handles a single character set, and we provide
-   wrappers for the wchar_t functionality we use.  */
+   Finally, some systems do not have iconv, or are really broken
+   (e.g., Solaris, which almost has all of this working, but where
+   just enough is broken to make it too hard to use).  Here we provide
+   a phony iconv which only handles a single character set, and we
+   provide wrappers for the wchar_t functionality we use.  */
 
 
-#define INTERMEDIATE_ENCODING "wchar_t"
-
 #if defined (HAVE_ICONV)
 #include <iconv.h>
 #else
@@ -45,9 +49,15 @@
 #define PHONY_ICONV
 #endif
 
-/* We use "btowc" as a sentinel to detect functioning wchar_t
-   support.  */
-#if defined (HAVE_ICONV) && defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC)
+/* We use "btowc" as a sentinel to detect functioning wchar_t support.
+   We check for either __STDC_ISO_10646__ or a new-enough libiconv in
+   order to ensure we can convert to and from wchar_t.  We choose
+   libiconv version 0x10D because it was reported that earlier
+   versions do not always accept "wchar_t" as an encoding
+   argument.  */
+#if defined (HAVE_ICONV) && defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC) \
+  && (defined (__STDC_ISO_10646__) \
+      || (defined (_LIBICONV_VERSION) && _LIBICONV_VERSION >= 0x10D))
 
 #include <wchar.h>
 #include <wctype.h>
@@ -63,7 +73,32 @@ typedef wint_t gdb_wint_t;
 
 #define LCST(X) L ## X
 
+/* If __STDC_ISO_10646__ is defined, then the host wchar_t is UCS-4.
+   We exploit this fact in the hope that there are hosts that define
+   this but which do not support "wchar_t" as an encoding argument to
+   iconv_open.  We put the endianness into the encoding name to avoid
+   hosts that emit a BOM when the unadorned name is used.  */
+#if defined (__STDC_ISO_10646__)
+#if WORDS_BIGENDIAN
+#define INTERMEDIATE_ENCODING "UCS-4BE"
 #else
+#define INTERMEDIATE_ENCODING "UCS-4LE"
+#endif
+#elif defined (_LIBICONV_VERSION) && _LIBICONV_VERSION >= 0x10D
+#define INTERMEDIATE_ENCODING "wchar_t"
+#else
+/* This shouldn't happen, because the earlier #if should have filtered
+   out this case.  */
+#error "Neither __STDC_ISO_10646__ nor _LIBICONV_VERSION defined"
+#endif
+
+#else
+
+/* If we got here and have wchar_t support, we might be on a system
+   with some problem.  So, we just disable everything.  */
+#if !(defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC))
+#define PHONY_ICONV
+#endif
 
 typedef char gdb_wchar_t;
 typedef int gdb_wint_t;
@@ -80,8 +115,9 @@ typedef int gdb_wint_t;
    narrow encoding as our intermediate encoding.  However, if we are
    also providing a phony iconv, we might as well just stick with
    "wchar_t".  */
-#ifndef PHONY_ICONV
-#undef INTERMEDIATE_ENCODING
+#ifdef PHONY_ICONV
+#define INTERMEDIATE_ENCODING "wchar_t"
+#else
 #define INTERMEDIATE_ENCODING host_charset ()
 #endif
 

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> Here's a new version that should disable iconv on this platform, among
Tom> others.

Hi Pierre.  Did this patch work for you?

Tom

RE: Your INTERMEDIATE_ENCODING patch for Solaris

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : Monday, August 30, 2010 8:01 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: Your INTERMEDIATE_ENCODING patch for Solaris
> 
> >>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:
> 
> Tom> Here's a new version that should disable iconv on this platform,
> among
> Tom> others.
> 
> Hi Pierre.  Did this patch work for you?

  Hi Tom,
  sorry for the delay:

  here are a few results:

Test machine #1: 
uname -a output:
SunOS opensolaris 5.11 snv_111b i86pc i386 i86pc Solaris

With your patch, gdb compiles fine, 

gdbcvs/build-norm/gdb$ grep ICONV *.h
config.h:#define HAVE_ICONV 1
config.h:/* #undef HAVE_ICONVLIST */
config.h:/* #undef HAVE_LIBICONVLIST */
config.h:#define ICONV_CONST const

./gdb ./gdb
(top-gdb) p version
$1 = "7.2.50.20100831-cvs"

after replacing \r\n by \r+\n in charset.exp, I get:
.../gdb/testsuite$ gmake check RUNTESTFLAGS=charset.exp
                === gdb Summary ===

# of expected passes            111
# of unexpected failures        50
(Do you want the detailed output?)

(Without patch, but the same modified charset.exp,
the printing of version fails
I get:
                === gdb Summary ===

# of expected passes            74
# of unexpected failures        87
)

Test machine #2: 
uname -a output:
uname -a
SunOS muller 5.11 snv_111b sun4v sparc SUNW,Sun-Fire-T200 Solaris
gdb/buildcvs/gdb$ ./gdb ./gdb
(top-gdb) p version
$1 = "7.2.50.20100831-cvs"
(top-gdb) inf func iconv
All functions matching regular expression "iconv":

File ../../src/gdb/charset.c:
size_t phony_iconv(int, const char **, size_t *, char **, size_t *);
int phony_iconv_close(int);
int phony_iconv_open(const char *, const char *);
static void cleanup_iconv(void *);
(top-gdb) q
:/gdb/buildcvs/gdb$ grep ICONV *.h
config.h:/* #undef HAVE_ICONV */
config.h:/* #undef HAVE_ICONVLIST */
config.h:/* #undef HAVE_LIBICONVLIST */
config.h:/* #undef ICONV_CONST */

So apparently on Sparc, the C library iconv is not found,
despite the fact that it is in libc.so:
/lib$ objdump -T libc.so |grep iconv
0005f768 g    DF .text  00000034  SUNW_0.8    .protected iconv
0005f6e8 g    DF .text  00000080  SUNW_0.8    .protected iconv_close
0005f000 g    DF .text  000000b4  SUNW_0.8    .protected iconv_open

  After searching, it appears that the problem comes from the fact
that /usr/local/include is searched before /usr/include,
while /usr/local/lib is not searched at all for libraries.

 /usr/local/include/iconv.h contains the /usr/local/lib/libiconv.so
header.
  Compilation without -liconv fails because
/usr/local/include/iconv.h has:
#define iconv libiconv
but libiconv is not found in c library.
 and the second compilation with "-liconv" option fails
because /usr/local/lib is not in the library search path.
  Ugly, no?
For that machine, the output did not change as none of the
ICONV macros are set.

Test machine #3 x86_64 prcoessor:
uname -a
SunOS muller 5.11 snv_111b i86pc i386 i86pc Solaris
gdb/build-64/gdb$ grep ICONV *.h
config.h:#define HAVE_ICONV 1
config.h:/* #undef HAVE_ICONVLIST */
config.h:/* #undef HAVE_LIBICONVLIST */
config.h:#define ICONV_CONST const

So this one uses c library iconv.
(top-gdb) p version
$1 = <error reading variable>
(top-gdb) inf fun iconv
All functions matching regular expression "iconv":

File ../../src/gdb/charset.c:
static void cleanup_iconv(void *);

Non-debugging symbols:
0x0000000000514860  iconv_open
0x0000000000514860  iconv_open@plt
0x0000000000514870  iconv_close
0x0000000000514870  iconv_close@plt
0x0000000000514880  iconv
0x0000000000514880  iconv@plt
(top-gdb) set host-charset UTF-8
(top-gdb) p version
$2 = "7.2.50.20100831-cvs"

 So here, the c library iconv functions are used,
but the default host-charset is set to ASCII which is 
not handled by that iconv :(

So I would like to have the default host and target 
charset changed to UTF-8.

It is of course possible to use:
gdb/build-64/gdb$ export LANG=en_US.UTF-8
in that case, the defaults are set correctly:
(top-gdb) p version
$1 = "7.2.50.20100831-cvs"
(top-gdb) show charset
The host character set is "auto; currently UTF-8".
The target character set is "auto; currently UTF-8".
The target wide character set is "auto; currently UTF-32".

  But I would still prefer if we changed also the default
to UTF-8 for solaris.

Pierre

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   After searching, it appears that the problem comes from the fact
Pierre> that /usr/local/include is searched before /usr/include,
Pierre> while /usr/local/lib is not searched at all for libraries.

Yuck.

This setup seems unfortunate but I don't think there is much gdb should
do about it.

Pierre> Test machine #3 x86_64 prcoessor:
[...]
Pierre> So this one uses c library iconv.
Pierre> (top-gdb) p version
Pierre> $1 = <error reading variable>
Pierre> (top-gdb) inf fun iconv
Pierre> All functions matching regular expression "iconv":

Pierre>  So here, the c library iconv functions are used,
Pierre> but the default host-charset is set to ASCII which is 
Pierre> not handled by that iconv :(

I am not sure how this happens with the patch I posted.
Does this machine define __STDC_ISO_10646__?
Or did I somehow get the #if logic wrong?

Pierre> So I would like to have the default host and target 
Pierre> charset changed to UTF-8.

What does nl_langinfo(CODESET) return on this sytem?
If you remove the special "646" case, does it work?

Tom

RE: Your INTERMEDIATE_ENCODING patch for Solaris

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : Tuesday, August 31, 2010 6:47 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: Your INTERMEDIATE_ENCODING patch for Solaris
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
> writes:
> 
> Pierre>   After searching, it appears that the problem comes from the
> fact
> Pierre> that /usr/local/include is searched before /usr/include,
> Pierre> while /usr/local/lib is not searched at all for libraries.
> 
> Yuck.
> 
> This setup seems unfortunate but I don't think there is much gdb should
> do about it.

  I agree with you here :(
 
> Pierre> Test machine #3 x86_64 prcoessor:
> [...]
> Pierre> So this one uses c library iconv.
> Pierre> (top-gdb) p version
> Pierre> $1 = <error reading variable>
> Pierre> (top-gdb) inf fun iconv
> Pierre> All functions matching regular expression "iconv":
> 
> Pierre>  So here, the c library iconv functions are used,
> Pierre> but the default host-charset is set to ASCII which is
> Pierre> not handled by that iconv :(
> 
> I am not sure how this happens with the patch I posted.
> Does this machine define __STDC_ISO_10646__?
> Or did I somehow get the #if logic wrong?

 
> Pierre> So I would like to have the default host and target
> Pierre> charset changed to UTF-8.
> 
> What does nl_langinfo(CODESET) return on this sytem?
> If you remove the special "646" case, does it work?

  the ASCII charset is indeed set by the check to
"646".

  If I replace this by (only the "646" specific change is listed):
Index: charset.c
===================================================================
RCS file: /cvs/src/src/gdb/charset.c,v
retrieving revision 1.34
diff -u -p -r1.34 charset.c
--- charset.c   24 Jun 2010 18:24:03 -0000      1.34
+++ charset.c   1 Sep 2010 07:28:23 -0000
@@ -928,7 +931,9 @@ _initialize_charset (void)
   /* Solaris will return `646' here -- but the Solaris iconv then
      does not accept this.  Darwin (and maybe FreeBSD) may return "" here,
      which GNU libiconv doesn't like (infinite loop).  */
-  if (!strcmp (auto_host_charset_name, "646") || !*auto_host_charset_name)
+  if (!strcmp (auto_host_charset_name, "646"))
+    auto_host_charset_name = "UTF-8";
+  else if (!*auto_host_charset_name)
     auto_host_charset_name = "ASCII";
   auto_target_charset_name = auto_host_charset_name;
 #elif defined (USE_WIN32API)

I do get a default of '"UTF-8" for Open Solaris machines,
but I don't know if other systems might also return "646",
but not like the "UTF-8" choice.

  In any case, having a LANG environment variable set to xx_XX.UTF-8
also allows to get a nicely working gdb.

Pierre

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

Pierre> So here, the c library iconv functions are used,
Pierre> but the default host-charset is set to ASCII which is
Pierre> not handled by that iconv :(

Tom> I am not sure how this happens with the patch I posted.
Tom> Does this machine define __STDC_ISO_10646__?
Tom> Or did I somehow get the #if logic wrong?

I didn't see an answer to this.
I still don't understand how this particular build is using iconv and
not the phony iconv.

Pierre> I do get a default of '"UTF-8" for Open Solaris machines,
Pierre> but I don't know if other systems might also return "646",
Pierre> but not like the "UTF-8" choice.

AFAIK only Solaris does this.  It seems to me that UTF-8 is not correct,
though -- it may vary by locale.

Maybe we need to go even further and blacklist Solaris from ever using
iconv :-(.  It would help to know the answer to the above.

Tom

RE: Your INTERMEDIATE_ENCODING patch for Solaris

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : Thursday, September 02, 2010 12:22 AM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: Your INTERMEDIATE_ENCODING patch for Solaris
> 
> Pierre> So here, the c library iconv functions are used,
> Pierre> but the default host-charset is set to ASCII which is
> Pierre> not handled by that iconv :(
> 
> Tom> I am not sure how this happens with the patch I posted.
> Tom> Does this machine define __STDC_ISO_10646__?
> Tom> Or did I somehow get the #if logic wrong?
> 
> I didn't see an answer to this.
> I still don't understand how this particular build is using iconv and
> not the phony iconv.

  Sorry, after investigation it appears that:
__STDC_ISO_10646__ is not in any headers under /usr/include
but both HAVE_WCHAR_T and HAVE_BTOWC are defined,
so that with you patch PHONY_ICONV macro doesn't get set.

  Does this answer your question?

> Pierre> I do get a default of '"UTF-8" for Open Solaris machines,
> Pierre> but I don't know if other systems might also return "646",
> Pierre> but not like the "UTF-8" choice.
> 
> AFAIK only Solaris does this.  It seems to me that UTF-8 is not
> correct,
> though -- it may vary by locale.
> 
> Maybe we need to go even further and blacklist Solaris from ever using
> iconv :-(.  It would help to know the answer to the above.

  Probably just define PHONY_ICONV if the first set of conditions is
not met (by the way, I am not sure the libiconv version test
that we added is really pertinent.)

Pierre

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

Pierre>   Probably just define PHONY_ICONV if the first set of conditions is
Pierre> not met (by the way, I am not sure the libiconv version test
Pierre> that we added is really pertinent.)

Thanks.  I think I got the logic wrong:

+/* If we got here and have wchar_t support, we might be on a system
+   with some problem.  So, we just disable everything.  */
+#if !(defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC))
+#define PHONY_ICONV
+#endif


I think that "!" is wrong -- it even contradicts the comment.
I'm sorry to keep bothering you about this, but could you remove that
"!" and try again?

Tom

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Pierre> Probably just define PHONY_ICONV if the first set of conditions is
Pierre> not met (by the way, I am not sure the libiconv version test
Pierre> that we added is really pertinent.)

Tom> Thanks.  I think I got the logic wrong:

Tom> +/* If we got here and have wchar_t support, we might be on a system
Tom> +   with some problem.  So, we just disable everything.  */
Tom> +#if !(defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC))
Tom> +#define PHONY_ICONV
Tom> +#endif

Tom> I think that "!" is wrong -- it even contradicts the comment.
Tom> I'm sorry to keep bothering you about this, but could you remove that
Tom> "!" and try again?

Any word on this?
I would like to commit this patch if it seems ok.

Tom

RE: Your INTERMEDIATE_ENCODING patch for Solaris

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : Tuesday, September 14, 2010 10:57 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: Your INTERMEDIATE_ENCODING patch for Solaris
> 
> >>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:
> 
> Pierre> Probably just define PHONY_ICONV if the first set of conditions
> is
> Pierre> not met (by the way, I am not sure the libiconv version test
> Pierre> that we added is really pertinent.)
> 
> Tom> Thanks.  I think I got the logic wrong:
> 
> Tom> +/* If we got here and have wchar_t support, we might be on a
> system
> Tom> +   with some problem.  So, we just disable everything.  */
> Tom> +#if !(defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC))
> Tom> +#define PHONY_ICONV
> Tom> +#endif
> 
> Tom> I think that "!" is wrong -- it even contradicts the comment.
> Tom> I'm sorry to keep bothering you about this, but could you remove
> that
> Tom> "!" and try again?
> 
> Any word on this?
> I would like to commit this patch if it seems ok.

  I can at least confirm that for
Sparc OpenSolaris 5.11 snv_111b it does
disable use of iconv functions from libc.so.1

  So, at least for me, this should be modified
that way. 

 Pierre

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   I can at least confirm that for
Pierre> Sparc OpenSolaris 5.11 snv_111b it does
Pierre> disable use of iconv functions from libc.so.1

Pierre>   So, at least for me, this should be modified
Pierre> that way. 

Great.  Here is what I am checking in.

Tom

2010-09-15  Tom Tromey  <tromey@redhat.com>

	* charset.c (iconv_open): New define.
	(iconv): Likewise.
	(iconv_close): Likewise.
	(phony_iconv_open): Add "phony_" prefix.
	(phony_iconv_close): Likewise.
	(phony_iconv): Likewise.
	* gdb_wchar.h: Check _LIBICONV_VERSION, __STDC_ISO_10646__.
	Change how INTERMEDIATE_ENCODING is defined.

Index: charset.c
===================================================================
RCS file: /cvs/src/src/gdb/charset.c,v
retrieving revision 1.34
diff -u -r1.34 charset.c
--- charset.c	24 Jun 2010 18:24:03 -0000	1.34
+++ charset.c	15 Sep 2010 20:03:02 -0000
@@ -90,8 +90,11 @@
 #undef iconv_t
 #define iconv_t int
 #undef iconv_open
+#define iconv_open phony_iconv_open
 #undef iconv
+#define iconv phony_iconv
 #undef iconv_close
+#define iconv_close phony_iconv_close
 
 #undef ICONV_CONST
 #define ICONV_CONST const
@@ -106,7 +109,7 @@
 #endif
 
 iconv_t
-iconv_open (const char *to, const char *from)
+phony_iconv_open (const char *to, const char *from)
 {
   /* We allow conversions from UTF-32BE, wchar_t, and the host charset.
      We allow conversions to wchar_t and the host charset.  */
@@ -122,14 +125,14 @@
 }
 
 int
-iconv_close (iconv_t arg)
+phony_iconv_close (iconv_t arg)
 {
   return 0;
 }
 
 size_t
-iconv (iconv_t utf_flag, const char **inbuf, size_t *inbytesleft,
-       char **outbuf, size_t *outbytesleft)
+phony_iconv (iconv_t utf_flag, const char **inbuf, size_t *inbytesleft,
+	     char **outbuf, size_t *outbytesleft)
 {
   if (utf_flag)
     {
Index: gdb_wchar.h
===================================================================
RCS file: /cvs/src/src/gdb/gdb_wchar.h,v
retrieving revision 1.3
diff -u -r1.3 gdb_wchar.h
--- gdb_wchar.h	1 Jan 2010 07:31:32 -0000	1.3
+++ gdb_wchar.h	15 Sep 2010 20:03:02 -0000
@@ -23,20 +23,24 @@
    
    Capable systems have the full suite: wchar_t support and iconv
    (perhaps via GNU libiconv).  On these machines, full functionality
-   is available.
+   is available.  Note that full functionality is dependent on us
+   being able to convert from an arbitrary encoding to wchar_t.  In
+   practice this means we look for __STDC_ISO_10646__ (where we know
+   the name of the wchar_t encoding) or GNU libiconv, where we can use
+   "wchar_t".
    
    DJGPP is known to have libiconv but not wchar_t support.  On
    systems like this, we use the narrow character functions.  The full
    functionality is available to the user, but many characters (those
    outside the narrow range) will be displayed as escapes.
    
-   Finally, some systems do not have iconv.  Here we provide a phony
-   iconv which only handles a single character set, and we provide
-   wrappers for the wchar_t functionality we use.  */
+   Finally, some systems do not have iconv, or are really broken
+   (e.g., Solaris, which almost has all of this working, but where
+   just enough is broken to make it too hard to use).  Here we provide
+   a phony iconv which only handles a single character set, and we
+   provide wrappers for the wchar_t functionality we use.  */
 
 
-#define INTERMEDIATE_ENCODING "wchar_t"
-
 #if defined (HAVE_ICONV)
 #include <iconv.h>
 #else
@@ -45,9 +49,15 @@
 #define PHONY_ICONV
 #endif
 
-/* We use "btowc" as a sentinel to detect functioning wchar_t
-   support.  */
-#if defined (HAVE_ICONV) && defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC)
+/* We use "btowc" as a sentinel to detect functioning wchar_t support.
+   We check for either __STDC_ISO_10646__ or a new-enough libiconv in
+   order to ensure we can convert to and from wchar_t.  We choose
+   libiconv version 0x10D because it was reported that earlier
+   versions do not always accept "wchar_t" as an encoding
+   argument.  */
+#if defined (HAVE_ICONV) && defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC) \
+  && (defined (__STDC_ISO_10646__) \
+      || (defined (_LIBICONV_VERSION) && _LIBICONV_VERSION >= 0x10D))
 
 #include <wchar.h>
 #include <wctype.h>
@@ -63,7 +73,32 @@
 
 #define LCST(X) L ## X
 
+/* If __STDC_ISO_10646__ is defined, then the host wchar_t is UCS-4.
+   We exploit this fact in the hope that there are hosts that define
+   this but which do not support "wchar_t" as an encoding argument to
+   iconv_open.  We put the endianness into the encoding name to avoid
+   hosts that emit a BOM when the unadorned name is used.  */
+#if defined (__STDC_ISO_10646__)
+#if WORDS_BIGENDIAN
+#define INTERMEDIATE_ENCODING "UCS-4BE"
 #else
+#define INTERMEDIATE_ENCODING "UCS-4LE"
+#endif
+#elif defined (_LIBICONV_VERSION) && _LIBICONV_VERSION >= 0x10D
+#define INTERMEDIATE_ENCODING "wchar_t"
+#else
+/* This shouldn't happen, because the earlier #if should have filtered
+   out this case.  */
+#error "Neither __STDC_ISO_10646__ nor _LIBICONV_VERSION defined"
+#endif
+
+#else
+
+/* If we got here and have wchar_t support, we might be on a system
+   with some problem.  So, we just disable everything.  */
+#if defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC)
+#define PHONY_ICONV
+#endif
 
 typedef char gdb_wchar_t;
 typedef int gdb_wint_t;
@@ -80,8 +115,9 @@
    narrow encoding as our intermediate encoding.  However, if we are
    also providing a phony iconv, we might as well just stick with
    "wchar_t".  */
-#ifndef PHONY_ICONV
-#undef INTERMEDIATE_ENCODING
+#ifdef PHONY_ICONV
+#define INTERMEDIATE_ENCODING "wchar_t"
+#else
 #define INTERMEDIATE_ENCODING host_charset ()
 #endif
 

RE: Your INTERMEDIATE_ENCODING patch for Solaris

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : Wednesday, September 15, 2010 10:18 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: Your INTERMEDIATE_ENCODING patch for Solaris
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
> writes:
> 
> Pierre>   I can at least confirm that for
> Pierre> Sparc OpenSolaris 5.11 snv_111b it does
> Pierre> disable use of iconv functions from libc.so.1
> 
> Pierre>   So, at least for me, this should be modified
> Pierre> that way.
> 
> Great.  Here is what I am checking in.

  Nice, but I was wondering if the
check of libiconv version is still needed.
  Wasn't it suggested by me? 
  I think that checking for the existence of 
_LIBICONV_VERSION macro should be enough,
unless you are really sure that older versions
do not work. 
  On systems that have an older GNU libiconv
version installed, this might disable the use of GNU libiconv
without a good reason.


Pierre

[patch] Regression on py-prettyprint.exp: print estring [Re: Your INTERMEDIATE_ENCODING patch for Solaris]

[Jan Kratochvil]

On Wed, 15 Sep 2010 22:18:07 +0200, Tom Tromey wrote:
> Great.  Here is what I am checking in.
> 
> 2010-09-15  Tom Tromey  <tromey@redhat.com>
> 
> 	* charset.c (iconv_open): New define.
> 	(iconv): Likewise.
> 	(iconv_close): Likewise.
> 	(phony_iconv_open): Add "phony_" prefix.
> 	(phony_iconv_close): Likewise.
> 	(phony_iconv): Likewise.
> 	* gdb_wchar.h: Check _LIBICONV_VERSION, __STDC_ISO_10646__.
> 	Change how INTERMEDIATE_ENCODING is defined.

562fabbaa40fe9e601eddc2e0ab3c8cc615cf9a3

This patch has a regression on all the tested OSes
{x86_64,x86_64-m32,i686}-fedora{12,13,14snapshot}-linux-gnu:

(gdb) print estring
$7 = "embedded x\201\202\203\204"
PASS: gdb.python/py-prettyprint.exp: print estring
->
(gdb) print estring
$7 = "embedded \201\202\203\204"
(gdb) FAIL: gdb.python/py-prettyprint.exp: print estring

due to the change:
INTERMEDIATE_ENCODING = "wchar_t"
->
INTERMEDIATE_ENCODING = "UCS-4LE"

echo 'char s[]="ab\201";'|gcc -o 1.o -c -g -x c -;./gdb -nx -q -ex 'p s' -ex q ./1.o
$1 = "ab\201"
->
$1 = "a\201"

The fix has no regressions on
{x86_64,x86_64-m32,i686}-fedora14snapshot-linux-gnu.

Test OpenSolaris 2009.06 snv_111b X86 shows no problem even with FSF GDB HEAD;
there was no libiconv so charset.exp is UNTESTED.
(I did not try GNU libiconv on OpenSolaris).

The fix seems generally correct to me anyway.  The conversion may not stop
when it seens no more output space as the input may no longer produce any
output.  It is more questionable why wchar_t worked (and how is it different
in glibc iconv).


Thanks,
Jan


gdb/
2010-09-16  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* charset.c (wchar_iterate) <EILSEQ>: Return any possibly converted
	characters.

--- a/gdb/charset.c
+++ b/gdb/charset.c
@@ -648,8 +648,13 @@ wchar_iterate (struct wchar_iterator *iter,
 	  switch (errno)
 	    {
 	    case EILSEQ:
-	      /* Invalid input sequence.  Skip it, and let the caller
-		 know about it.  */
+	      /* Invalid input sequence.  We still might have converted a
+		 character; if so, return it.  */
+	      if (out_avail < out_request * sizeof (gdb_wchar_t))
+		break;
+	      
+	      /* Otherwise skip the first invalid character, and let the
+		 caller know about it.  */
 	      *out_result = wchar_iterate_invalid;
 	      *ptr = iter->input;
 	      *len = iter->width;

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Tom> Great.  Here is what I am checking in.

Pierre>   Nice, but I was wondering if the
Pierre> check of libiconv version is still needed.
Pierre>   Wasn't it suggested by me? 

According to the comment, versions of libiconv before 0x10D do not
support "wchar_t" as an argument to iconv_open.

I think you discovered this, but it is hard to remember.

Pierre>   I think that checking for the existence of 
Pierre> _LIBICONV_VERSION macro should be enough,
Pierre> unless you are really sure that older versions
Pierre> do not work. 
Pierre>   On systems that have an older GNU libiconv
Pierre> version installed, this might disable the use of GNU libiconv
Pierre> without a good reason.

I think it is better to fail safe:
If this code is disabled, the user gets reduced functionality.
If this code is erroneously enabled, users can't print strings at all.

Tom

Re: [patch] Regression on py-prettyprint.exp: print estring [Re: Your INTERMEDIATE_ENCODING patch for Solaris]

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> The fix seems generally correct to me anyway.  The conversion may
Jan> not stop when it seens no more output space as the input may no
Jan> longer produce any output.  It is more questionable why wchar_t
Jan> worked (and how is it different in glibc iconv).

Jan> 2010-09-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
Jan> 	* charset.c (wchar_iterate) <EILSEQ>: Return any possibly converted
Jan> 	characters.

Thanks, I agree this is correct.

Tom

Re: [patch] Regression on py-prettyprint.exp: print estring [Re: Your INTERMEDIATE_ENCODING patch for Solaris]

[Jan Kratochvil]

On Thu, 16 Sep 2010 21:17:02 +0200, Tom Tromey wrote:
> Jan> 2010-09-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
> Jan> 	* charset.c (wchar_iterate) <EILSEQ>: Return any possibly converted
> Jan> 	characters.
> 
> Thanks, I agree this is correct.

Checked-in:
	http://sourceware.org/ml/gdb-cvs/2010-09/msg00116.html


Thanks,
Jan

RE: Your INTERMEDIATE_ENCODING patch for Solaris

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : Thursday, September 16, 2010 8:31 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: Your INTERMEDIATE_ENCODING patch for Solaris
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
> writes:
> 
> Tom> Great.  Here is what I am checking in.
> 
> Pierre>   Nice, but I was wondering if the
> Pierre> check of libiconv version is still needed.
> Pierre>   Wasn't it suggested by me?
> 
> According to the comment, versions of libiconv before 0x10D do not
> support "wchar_t" as an argument to iconv_open.
> 
> I think you discovered this, but it is hard to remember.
  Yes, I wrote that in
http://sourceware.org/ml/gdb-patches/2010-08/msg00296.html
but I now fear that I made an error in my analysis at that time.
   Rereading my email lets me think that maybe
the Sparc did not link to GNU iconv library, but instead tried
to use the libc iconv functions.
   Troubles is that I lost access to that Sparc machine.
> Pierre>   I think that checking for the existence of
> Pierre> _LIBICONV_VERSION macro should be enough,
> Pierre> unless you are really sure that older versions
> Pierre> do not work.
> Pierre>   On systems that have an older GNU libiconv
> Pierre> version installed, this might disable the use of GNU libiconv
> Pierre> without a good reason.
> 
> I think it is better to fail safe:
> If this code is disabled, the user gets reduced functionality.
> If this code is erroneously enabled, users can't print strings at all.

  Would it be possible to test an old libiconv on some other platform?

Pierre

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   Would it be possible to test an old libiconv on some other platform?

I can't do this any time soon, but according to the libiconv NEWS file,
this feature was added in 1.5.  So, perhaps we can relax the version
check to 0x105.

Tom

RE: Your INTERMEDIATE_ENCODING patch for Solaris

[Pierre Muller]

 

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : Friday, September 17, 2010 4:13 AM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: Your INTERMEDIATE_ENCODING patch for Solaris
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
> writes:
> 
> Pierre>   Would it be possible to test an old libiconv on some other
> platform?
> 
> I can't do this any time soon, but according to the libiconv NEWS file,
> this feature was added in 1.5.  So, perhaps we can relax the version
> check to 0x105.

  I found the libiconv version 1.5
installed it on a x86 Open Solaris machine,
modified gdb_wchar.h to use _LIBICONV_VERSION >= 0x105
and got an gdb executable linked to libiconv version 1.5,
using --with-libiconv-prefix=/usr/local/src/test32
(the installation prefix I used for 1.5 libiconv).

  I tested charset.exp result on that executable,
and got a lot of failures:
                === gdb Summary ===

# of expected passes            68
# of unexpected failures        51
/usr/local/src/gdb/build32/gdb/testsuite/../../gdb/gdb version  7.2.50.20100916-
cvs -nw -nx

  while using 1.13.1 libiconv version, I got this:
                === gdb Summary ===

# of expected passes            159
# of unexpected failures        2

  But I was wondering if the problem is not coming from the
fact that for 1.5 libiconv find_charset_names function
directly calls 'iconv -l' (because iconvlist is not present
in this version of the library).
  The iconv that is called is the /usr/bin/iconv
not the one that comes with the installed library:
/usr/local/src/test32/bin/iconv.

  On the other hand, as iconvlist function is
not implemented in 1.5 version
'iconv -l' also doesn't work.
  This makes me believe that we should not attempt to
call 'iconv -l' if we link in GNU libiconv and no
iconvlist function exists.
  The list that is read in by 'iconv -l' call is the list
of libc iconv supported charsets in a format that is
mishandled by the code anyhow:
(top-gdb) set charset
gives this:
Requires an argument. Valid arguments are auto, fromcode-tocode., Some, of, those, 
code, set, names, have, aliases, which, are, case-insensitive, and, described, in, 
parentheses, following, the, canonical, name:, 5601, 646, (ASCII, US-ASCII
, US_ASCII, USASCII), 646da, 646de, 646en, 646es, 646fr, 646it, 646sv, 8859, 885
9-1, (ISO8859-1, ISO-8859-1, ISO8859_1, ISO_8859_1), 8859-10, (ISO8859-10, ISO88
(.... the list is much longer,)
 The output of the libc iconv is not parsed correctly
because aliases are within parenthesis that are not removed
and an introduction sentence is not recognized.

(top-gdb) set charset ASCII
Undefined item: "ASCII".
(top-gdb) set charset (ASCII
Cannot convert between character sets `UTF-32' and `(ASCII'
(top-gdb) set charset US_ASCII
Cannot convert between character sets `UTF-32' and `US_ASCII'
(top-gdb) set charset US-ASCII
(top-gdb) p version
$1 = "7.2.50.20100916-cvs"

So you can see:
 'ASCII' is refused by GDB because not in the 'wrong' list it
created by 'iconv -l' call.
 '(ASCII' is accepted by GDB, but refused by iconv_open
 'US_ASCII' is accepted by GDB, but refused by iconv_open,
probably because 1.5 GNU libiconv does not recognize that alias,
whereas 'US-ASCII' is accepted by GDB, and by iconv_open!

  That would mean that we should either:
 1) only accept libiconv if it also has iconvlist (1.8 already has iconvlist,
but there are some changes recorded in the ChangeLog after that).

or
 2) not call 'iconv -l' if libiconv has no iconvlist function
and use only the standard list defined.

  I also downloaded version 1.8 of libiconv,
and checked GDB linked to that version:
the results of charset.exp are the same as 1.13.1 (2 FAILs)

Pierre

PS: Support of libc iconv for Solaris could probably be 
enhanced by a better parsing of 'iconv -l' output...

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

>>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr> writes:

Pierre>   I found the libiconv version 1.5
Pierre> installed it on a x86 Open Solaris machine,
Pierre> modified gdb_wchar.h to use _LIBICONV_VERSION >= 0x105
Pierre> and got an gdb executable linked to libiconv version 1.5,
Pierre> using --with-libiconv-prefix=/usr/local/src/test32
Pierre> (the installation prefix I used for 1.5 libiconv).
Pierre>   I tested charset.exp result on that executable,
Pierre> and got a lot of failures:
[...]

Pierre>   But I was wondering if the problem is not coming from the
Pierre> fact that for 1.5 libiconv find_charset_names function
Pierre> directly calls 'iconv -l' (because iconvlist is not present
Pierre> in this version of the library).

Ouch.

Pierre>   I also downloaded version 1.8 of libiconv,
Pierre> and checked GDB linked to that version:
Pierre> the results of charset.exp are the same as 1.13.1 (2 FAILs)

Yeah, how about we make x0108 the minimal acceptable version?
It was released in 2002, that seems plenty old to me.

If you agree, I will make the change.

Pierre> PS: Support of libc iconv for Solaris could probably be 
Pierre> enhanced by a better parsing of 'iconv -l' output...

I think there isn't much reason to do it, since we're planning to avoid
Solaris iconv completely.  If somebody wants to make the effort, though,
it is fine by me.

Tom

RE: Your INTERMEDIATE_ENCODING patch for Solaris

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tom Tromey
> Envoyé : Wednesday, September 22, 2010 10:57 PM
> À : Pierre Muller
> Cc : gdb-patches@sourceware.org
> Objet : Re: Your INTERMEDIATE_ENCODING patch for Solaris
> 
> >>>>> "Pierre" == Pierre Muller <pierre.muller@ics-cnrs.unistra.fr>
> writes:
> 
> Pierre>   I found the libiconv version 1.5
> Pierre> installed it on a x86 Open Solaris machine,
> Pierre> modified gdb_wchar.h to use _LIBICONV_VERSION >= 0x105
> Pierre> and got an gdb executable linked to libiconv version 1.5,
> Pierre> using --with-libiconv-prefix=/usr/local/src/test32
> Pierre> (the installation prefix I used for 1.5 libiconv).
> Pierre>   I tested charset.exp result on that executable,
> Pierre> and got a lot of failures:
> [...]
> 
> Pierre>   But I was wondering if the problem is not coming from the
> Pierre> fact that for 1.5 libiconv find_charset_names function
> Pierre> directly calls 'iconv -l' (because iconvlist is not present
> Pierre> in this version of the library).
> 
> Ouch.
> 
> Pierre>   I also downloaded version 1.8 of libiconv,
> Pierre> and checked GDB linked to that version:
> Pierre> the results of charset.exp are the same as 1.13.1 (2 FAILs)
> 
> Yeah, how about we make x0108 the minimal acceptable version?
> It was released in 2002, that seems plenty old to me.
> 
> If you agree, I will make the change.

   I agree, furthermore HAVE_LIBICONVLIST
should always be set in config.h in that case,
which avoids the direct call to 'iconv -l'.
 
> Pierre> PS: Support of libc iconv for Solaris could probably be
> Pierre> enhanced by a better parsing of 'iconv -l' output...
> 
> I think there isn't much reason to do it, since we're planning to avoid
> Solaris iconv completely.  If somebody wants to make the effort,
> though,
> it is fine by me.

   Not sure I will have time for that...

Pierre

Re: Your INTERMEDIATE_ENCODING patch for Solaris

[Tom Tromey]

Tom> Yeah, how about we make x0108 the minimal acceptable version?
Tom> It was released in 2002, that seems plenty old to me.

Tom> If you agree, I will make the change.

Pierre>    I agree, furthermore HAVE_LIBICONVLIST
Pierre> should always be set in config.h in that case,
Pierre> which avoids the direct call to 'iconv -l'.
 
Here is the patch I am checking in.

Thank you for all the testing you did for this.

Tom

2010-09-27  Tom Tromey  <tromey@redhat.com>

	* gdb_wchar.h: Change minimum libiconv to 0x108.

Index: gdb_wchar.h
===================================================================
RCS file: /cvs/src/src/gdb/gdb_wchar.h,v
retrieving revision 1.4
diff -u -r1.4 gdb_wchar.h
--- gdb_wchar.h	15 Sep 2010 20:18:47 -0000	1.4
+++ gdb_wchar.h	27 Sep 2010 17:21:10 -0000
@@ -52,12 +52,11 @@
 /* We use "btowc" as a sentinel to detect functioning wchar_t support.
    We check for either __STDC_ISO_10646__ or a new-enough libiconv in
    order to ensure we can convert to and from wchar_t.  We choose
-   libiconv version 0x10D because it was reported that earlier
-   versions do not always accept "wchar_t" as an encoding
-   argument.  */
+   libiconv version 0x108 because it is the first version with
+   iconvlist.  */
 #if defined (HAVE_ICONV) && defined (HAVE_WCHAR_H) && defined (HAVE_BTOWC) \
   && (defined (__STDC_ISO_10646__) \
-      || (defined (_LIBICONV_VERSION) && _LIBICONV_VERSION >= 0x10D))
+      || (defined (_LIBICONV_VERSION) && _LIBICONV_VERSION >= 0x108))
 
 #include <wchar.h>
 #include <wctype.h>
@@ -84,7 +83,7 @@
 #else
 #define INTERMEDIATE_ENCODING "UCS-4LE"
 #endif
-#elif defined (_LIBICONV_VERSION) && _LIBICONV_VERSION >= 0x10D
+#elif defined (_LIBICONV_VERSION) && _LIBICONV_VERSION >= 0x108
 #define INTERMEDIATE_ENCODING "wchar_t"
 #else
 /* This shouldn't happen, because the earlier #if should have filtered