Re: [PATCH] libstdc++: implement locale support for AIX

[Jonathan Wakely <jwakely@redhat.com>]

On 22/01/21 12:04 +0100, Rainer Orth wrote:
>why?  I've just double-checked the OpenGroup pages: all of the functions
>listed as XPG7 above were part of IEEE 1003.1-2008, just some of them
>have Technical Corrigenda applied.  IIUC IEEE 1003.1-2017 is just a
>revision of the -2008 standard, not a new issue (XPG8 or something).

Technically, the 2008 standard has been withdrawn and replaced by
2017. Since the content is the same, it seems more correct to refer to
the current standard (even if Solaris only documents support for the
2008 edition, if it also implements the corrigenda then it conforms to
2017 even if it doesn't document that).

But as I said, a shorter, more memorable name like "xpg7" or just
"posix" might be preferable anyway.

>> it would be better to avoid all these #ifdef.
>> Some are still needed as for example only the last version of AIX have
>> strftime_l.
>
>Then that version doesn't conform to XPG7 and shouldn't use that clocale
>variant.  Until we have a clearer understanding of the variation here,
>I'd argue that only P1003.1-2008 conforming OSes should use
>ieee_1003.1-2008, rather than creating an impenetrable maze of #ifdefs
>for all sorts of partial implementations.
>
>>> As for the BSD group, I suggest to have one representative configure
>>>  test (for localeconv_l perhaps) and then use an appropriate name for the
>>> group as a whole.  Again, this will most likely be an all-or-nothing
>>> thing.
>>
>> I'm not sure this is really all-or-nothing for these. Maybe strtof_l and cie
>> can be grouped by. But the 3 others are really different. Linux have wcsftime_l
>> but not the others. AIX avec none. BSD have all.
>
>TBH, I don't care about Linux here: it will continue to use the gnu
>variant anyway.  Besides, since the patch will have to work on targets
>without wcsftime_l and the other BSD functions, I don't see any harm in
>not using one non-standard one of them although it's present.

I agree that GNU/Linux will continue to use the gnu model, but it can
still be usable as a useful extra test of the code's portablility,
since it implements everything in XPG7 (and more). We shouldn't spend
any effort doing linux-specific changes in this new model, since they
won't be used in practice. But if it works "out of the box" with no
tweaks, then that is useful for testing.

>>> Besides, your configure tests are way too complicated: just use
>>> AC_CHECK_FUNCS doing a link test and be done with it.
>>
>> Sadly, you can't pass includes to AC_CHECK_FUNCS. That's why I had to do
>> that. I've made a first version with AC_CHECK_DECLS which allows extra
>> headers, but it didn't work too. I might know why though.
>
>Why would you need to?  AC_CHECK_FUNCS just perform a link test to check
>if the functions are present in libc; no need to have a declaration
>present.

Right, I wondered about this too.

>>> In a similar vein, configure.ac already has
>>> AC_CHECK_HEADERS([xlocale.h]).  Rather than hardcoding the existance of
>>> the header based on the configure triple, just use the existing
>>> HAVE_XLOCALE_H.  This ways, things will simply fall into place for
>>> e.g. NetBSD, OpenBSD and possibly others.
>>
>> Right, I'll make the change. Thanks !
>>
>>> > 4) ctype_configure_char.cc
>>> > I've some troubles knowing what is supposed to be implemented on this file.
>>> > I don't really understand the part with setlocale which appears in many
>>> > os. When I'm adding it, some tests start failing, some start working...
>>> > Moreover, on Linux, if I understand correctly, there is some optimizations
>>> > based on classic_table(), _M_toupper and _M_tolower. Could you confirm
>>> > that it's only useful on Linux ?
>>>
>>> I don't know myself.  However, when trying the first version of your
>>> patch (augmented to compile on Solaris), the corresponding change to the
>>> solaris file made no difference in test results.
>>
>> I might have found the correct code since yesterday's mail. The problem seems
>> to come from _M_c_locale_ctype initialization. With locale support, it must be
>> _S_clone_c_locale(__cloc), without it, it must be the default locale which
>> ends up
>> being "C". I might push a newer patch this afternoon, with the correct code.
>
>Nice; I'll certainly give it a whirl!
>
>>> > Feel free to try in on other OS. But I've made modifications only for AIX and
>>> > Linux, as I can test the other ones.
>>>
>>> While reading through the patch, I saw that in two places you still use
>>> __DragonFly__ || __FreeBSD__ tests.  For one, it's hard to tell what
>>> feature they are really about, besides they will require fiddling with
>>> e.g. for other BSDs.  Please use a descriptive macro which says which
>>> difference this is about.
>>
>> Right, because I don't know how to handle them (and I've forgotten to ask
>> for it...).
>> The first is for typedef __c_locale. It seems to be int* instead of locale_t.
>> Could you confirm that this is wanted and mandatory ?
>> The second is in about some functions in ctype_members.cc which are
>> defined in config/os/../ctype_inlines.h for FreeBSD and Dragonfly. Someone
>> has to confirm that it can be merged with the new code, or if this is mandatory.
>
>That someone would most likely be Jonathan ;-)

I'll have to do some digging and refresh my memory of how all the
different clocale models work, which probably isn't going to happen
until stage 1.