From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.CeBiTec.Uni-Bielefeld.DE (smtp.CeBiTec.Uni-Bielefeld.DE [129.70.160.84]) by sourceware.org (Postfix) with ESMTPS id EBBA6398C818; Fri, 22 Jan 2021 11:04:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org EBBA6398C818 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=CeBiTec.Uni-Bielefeld.DE Authentication-Results: sourceware.org; spf=none smtp.mailfrom=ro@cebitec.uni-bielefeld.de Received: from localhost (localhost [127.0.0.1]) by smtp.CeBiTec.Uni-Bielefeld.DE (Postfix) with ESMTP id 2961B83C0; Fri, 22 Jan 2021 12:04:24 +0100 (CET) X-Virus-Scanned: amavisd-new at CeBiTec.Uni-Bielefeld.DE Received: from smtp.CeBiTec.Uni-Bielefeld.DE ([127.0.0.1]) by localhost (smtp.cebitec.uni-bielefeld.de [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id dOLH0Cs5wK77; Fri, 22 Jan 2021 12:04:23 +0100 (CET) Received: from manam.CeBiTec.Uni-Bielefeld.DE (p50855c8f.dip0.t-ipconnect.de [80.133.92.143]) by smtp.CeBiTec.Uni-Bielefeld.DE (Postfix) with ESMTPSA id 3994F82CA; Fri, 22 Jan 2021 12:04:23 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=CeBiTec.Uni-Bielefeld.DE; s=20200306; t=1611313463; bh=qUSZJl42EySCSZS774gfSOO/A499exUDdJI6cx98WL4=; h=From:To:Cc:Subject:References:Date:In-Reply-To:From; b=bL2SNSFD7wzp5xtrCwtRgnwG1l2uEhbhvILK6KKLaR9PcnnazO3B0lGHwdkZwBXPh lVQyQBbgrTUeiusS4YA099fp+OkpDKGGBnAByaG3RpJZNwayIAi5WuLXY6oe/4N9TJ dOlEX8BItTuhASAAJPLK5UJpQWtj8ZiATH3mBWYxJyvu2JtpOwkx8zBlWNSOnIUrKJ MVOEEPi2pvNRfe3afSOsjX0ta6DnSjcm+cDOSkm4S15T5UK8YeBam3KnSlJnvt2jxC x4H+rq5ezNvShvpAiH9nukp1qOVL46SHMlanVq6FjxHZsTDz5nmz7JqnyPyY359NkT NXYRbUvOl/Y6g== From: Rainer Orth To: "CHIGOT, CLEMENT" Cc: libstdc++ , David Edelsohn , Jonathan Wakely , David Edelsohn via Gcc-patches Subject: Re: [PATCH] libstdc++: implement locale support for AIX References: <20210111154058.GB21410@redhat.com> <20210112152505.GE21410@redhat.com> Date: Fri, 22 Jan 2021 12:04:22 +0100 In-Reply-To: (CLEMENT CHIGOT's message of "Fri, 22 Jan 2021 09:57:08 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1.90 (usg-unix-v) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-3789.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libstdc++@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libstdc++ mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Jan 2021 11:04:38 -0000 Hi Clement, >> > 3) POSIX 2017 and non-POSIX functions >> > Many of the *_l functions being used in GNU or dragonfly models aren't >> > POSIX 2008, but mainly POSIX 2017 or like strtof_l not POSIX at all. >> > However, there are really useful in the code, thus I've made a double >> > implementation based on "#ifdef HAVE_". Is it ok for you ? It's not really >> > POSIX 2008 but more POSIX 2008 with 2017 compatibility. >> > For the configure, I didn't find any better way to check each syscall, as >> > they all depend on different includes. Tell me if you have a better idea. >> >> First a general observation: there are two groups of functions you're >> testing for: >> >> * Pure BSD additions, not available in either POSIX.1, ISO C, or glibc: >> >> localeconv_l >> mbstowcs_l >> strtod_l >> strtof_l >> strtold_l >> wcsftime_l >> >> * Part of XPG7: >> >> iswctype_l >> strcoll_l >> strftime_l >> strxfrm_l >> towlower_l >> towupper_l >> wcscoll_l >> wcsxfrm_l >> wctype_l >> >> My suggestion would be not to have configure tests _GLIBCXX_HAVE_ >> for any of the second group at all: this is ieee_1003.1-2008, after all, >> so if some OS selects that clocale variant, it better implement all of >> those. If really need be, one could a configure check for those and >> error out if any is missing. This makes the code way more readable than >> trying to handle some hypothetical partial implementation. > > In this case, it would be better to call it ieee_1003.1-2017 but I agree 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). > 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. >> 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. >> 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 ;-) >> That said, I gave the new patch a try on Solaris 11.4. To get it to >> compile, I had to apply two changes that I'd mentioned (without an actual >> patch) when commenting on the first patch: >> >> * The C99 fields of struct lconv need _LCONV_C99 to be visible for >> C++11. >> >> * Some ctype macros need __bitmapsize = 15, as the generic clocale >> implementation uses. > > If I'm not mistaking, POSIX is only defining 11 bit for ctype. If we want > some optimizations we can have a define of bitmasksize or we can simply > fill the whole mask by setting bitmasksize=15 as in generic. > I don't know what's best. AFAIK the constants used in (or on Solaris) to describe the different character classes are just an implementation detail of the respective OS; certainly none of them are listed on the OpenGroup page for XPG7 . So OSes are free to implement this any way they wish, and the generic variant (and Solaris) show that 11 bits are not enough on some. Rainer -- ----------------------------------------------------------------------------- Rainer Orth, Center for Biotechnology, Bielefeld University