This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Support newlib as libc++'s C library [locale part]
ClosedPublic

Authored by jroelofs on Sep 17 2014, 10:45 AM.

Details

Summary

I'm going to split out @jfb's original patch into three separate ones to lubricate the review process:

  • locale stuff
  • cmath stuff
  • classic_table & ctype mask stuff

This patch covers the first one.

Diff Detail

Event Timeline

jroelofs updated this revision to Diff 13790.Sep 17 2014, 10:45 AM
jroelofs retitled this revision from to [libc++] Support newlib as libc++'s C library [locale part].
jroelofs updated this object.
jroelofs edited the test plan for this revision. (Show Details)
jroelofs added reviewers: jfb, danalbert, mclow.lists.
jroelofs added a subscriber: jfb.
jroelofs added a subscriber: Unknown Object (MLST).

+cfe-commits

mclow.lists edited edge metadata.Sep 17 2014, 11:15 AM

A general comment: How much of this file is the same as $LIBCXX/include/support/bionic/bionic_locale.h, and would it be beneficial/desirable to create a file that stubs out the locale routines and share it between those two implementations.

Other than that, this looks pretty good.
I like the lack of damage to <locale> a lot.

mclow.lists added inline comments.Sep 17 2014, 11:16 AM
include/support/newlib/xlocale.h
65

Comment lost the first time. Sigh.

this should be written as:

`static inline int isdigit_l(char c, locale_t)`

You can't use the second parameter if you don't name it.

jfb added inline comments.Sep 17 2014, 11:20 AM
include/locale
197

This is only done in .cpp files, putting it here means it leaks to users.

include/support/newlib/xlocale.h
28

It's only guarded out if __STRICT_ANSI__, which seems correct? It's a C extension AFAICT, it's not in C11.

isascii.c defines it as return return c >= 0 && c< 128; unconditionally, so it should still be linked in?

65

Cleaner to just not give a name to locale_t l here and below instead of (void) l;?

danalbert added inline comments.Sep 17 2014, 11:27 AM
include/locale
197

newlib's sys/cdefs.h seems to do this anyway. Talking with jroelofs on IRC, he's going to see if we can skip this check by changing newlib's sys/cdefs.h to #define some way to identify as newlib (that's where other libcs provide such a definition).

include/support/newlib/xlocale.h
37

Can't use nullptr here (need to be able to include from pre-C++11 code).

Yeah, it would be better to share the implementation of these between android and newlib. Where's the right place to put that kind of shared thing?

include/support/newlib/xlocale.h
65

Yeah.. brainfart.

jroelofs added inline comments.Sep 17 2014, 11:33 AM
include/support/newlib/xlocale.h
37

Good point. I'll change that to NULL for the time being.

In a separate patch I've got some "healthier" stubs which fix a few of the libc++ tests that want the C locale (i.e. localization/locales/locale/locale.statics/classic.pass.cpp ).

jroelofs added inline comments.Sep 17 2014, 11:34 AM
include/support/newlib/xlocale.h
37

The failure being the:

terminating with uncaught exception of type std::runtime_error: collate_byname<char>::collate_byname failed to construct for C

one.

danalbert added inline comments.Sep 17 2014, 11:42 AM
include/support/newlib/xlocale.h
37

Good idea. iirc even the basic streams don't work if the implementation can't support the C locale.

mclow.lists added inline comments.Sep 17 2014, 12:06 PM
include/support/newlib/xlocale.h
37

Note that libc++ emulates nullptr in C++03 mode.

67

Should all of these explicitly specify the global namespace?

return ::isdigit(c);

That's not good if isdigit is a macro, but I don't know if that is true for newlib.

jroelofs added inline comments.Sep 17 2014, 12:10 PM
include/support/newlib/xlocale.h
67

All of these are implemented as macros in newlib.

jroelofs updated this revision to Diff 13797.Sep 17 2014, 12:52 PM
jroelofs edited edge metadata.

Share implementation with android.

Pasted the wrong diff. Ignore the most recent one.

jfb added inline comments.Sep 17 2014, 1:13 PM
include/locale
197

I still don't think __has_include should be defined in a header, could you put a TODO about fixing the issue in newlib first?

include/support/newlib/xlocale.h
28

As noted before, I think newlib is correct here.

65

Should this be after you terminate the extern "C"?

Again, pasted the wrong diff. Ignore the most recent one.

danalbert added inline comments.Sep 17 2014, 1:15 PM
include/support/xlocale/xlocale.h
2

Can delete the android one and fix up the #includes now, right?

jroelofs updated this revision to Diff 13799.Sep 17 2014, 1:23 PM

Here's the actual "share with android" update.

jroelofs added inline comments.Sep 17 2014, 1:25 PM
include/support/xlocale/xlocale.h
165

Note that the type of base changed here. locale_bionic.h got this wrong before, and likewise for the other three.

jfb added inline comments.Sep 17 2014, 1:32 PM
include/support/newlib/xlocale.h
28

isascii ins't in C++11 either :-)

Should libc++ not use it at all, or use a special __isascii?

60

typo

jroelofs added inline comments.Sep 17 2014, 3:01 PM
include/locale
197

Discussion with newlib folks suggests that this check and include isn't necessary at all as ctime includes time.h which includes _ansi.h, which includes newlib.h. They might be convinced to add NEWLIB to sys/cdefs.h, but until then, we can get by with dropping this part of the patch.

jfb added inline comments.Sep 17 2014, 3:05 PM
include/locale
197

The thread:

https://sourceware.org/ml/newlib/2014/msg00455.html

This sounds good to me, though it might be an issue in headers where we try to minimize the number/size of headers included (locale is OK, but others may need _NEWLIB_VERSION and have to have an extra include until the newlib fix comes).

jroelofs added inline comments.Sep 17 2014, 3:24 PM
include/locale
195
include/support/newlib/xlocale.h
28

Not using it would mean lots of ugly newlib specific stuff in locale.cpp and __locale. Sounds like inline int __isascii(int c) { int isascii(int); return isascii(c); } is the way to go here.

60

good catch

jfb added inline comments.Sep 17 2014, 3:38 PM
include/locale
202

Can't you drop the above since ctime indirectly includes newlib.h already in the current version of newlib?

include/support/newlib/xlocale.h
28

Would you just leave __isascii in this header? Or fix the rest of libc++ and move it to a global header? That'll be tricky because at least glibc has #define __isascii(c).

jroelofs updated this revision to Diff 13805.Sep 17 2014, 3:44 PM

s/isascii/__isascii/ along with appropriate shims in include/support because isascii isn't supposed to be part of C++.

On second thought, this probably breaks OSX and any other platform that doesn't have support shims. I'm not sure of the best way to proceed on that.

jroelofs added inline comments.Sep 17 2014, 3:45 PM
include/support/newlib/xlocale.h
28

Oh. Yucky. __libcpp_isascii it is then....

jroelofs updated this revision to Diff 13806.Sep 17 2014, 4:06 PM

libcpp_isascii, rather than isascii

jfb added inline comments.Sep 17 2014, 4:19 PM
include/__locale
44 ↗(On Diff #13806)

Could you add a comment explaining this?

Also, it relies on the C library actually exporting isascii, which it would as long as it has non-strict extensions. I think it's a fair assumption, but still worth pointing out.

Shouldn't the forward declaration be extern "C"? If it's found at link time and not in a header then the current code won't work.

include/support/newlib/xlocale.h
31

Remove.

63

Typo still there.

jroelofs added inline comments.Sep 17 2014, 5:04 PM
include/__locale
44 ↗(On Diff #13806)

Humph. I'm not sure I can extern "C" the inner declaration. I think I have to give that linkage to the whole thing.

include/support/newlib/xlocale.h
31

oops

63

oops

jroelofs updated this revision to Diff 13809.Sep 17 2014, 5:04 PM

Address comments.

jfb edited edge metadata.Sep 17 2014, 5:17 PM

I'm ready to sign off on this after the typos are fixed, but I'd like someone like @mclow.lists to chime in since the __libcpp_isascii thing is kind of ugly (but I don't see a better way).

include/__locale
44 ↗(On Diff #13809)

Two typos :)

include/locale
199

FWIW newlib has yearly releases around Christmas time, so "a bit" may be "quite a bit" ;-)

jroelofs updated this revision to Diff 13811.Sep 17 2014, 5:42 PM
jroelofs edited edge metadata.

Fiiiix typos

It is kind of ugly; and I worry about isascii being a macro.
Do any libraries do that?

Note: I'm not objecting because it's ugly; the name is fine.

joerg added a subscriber: joerg.Sep 18 2014, 2:39 AM

Why don't we fix the real problem first and stop bothering with the symptoms. The isascii checks break the masks for any char with high bit set. We should IMO have a templated function map2index or so. Then we get:

size_t map2index<char>(char_type a) { return (unsigned char)a; }
size_t map2index<wchar_t>(char_type a) { return a; }

It looks like SunOS's isascii is a macro: http://docs.oracle.com/cd/E19683-01/816-0213/6m6ne383q/index.html

@joerg For the ctype<char> ones, I think that's fine, but I don't think that works for ctype<wchar_t> because classic_table has only at least 256 entries. That would get isascii out of include/__locale, and we can leave the ones in src/locale.cpp, which solves this messiness.

include/locale
199

oh boy, yeah. I didn't know their release schedule was that infrequent :)

danalbert edited edge metadata.Sep 18 2014, 8:48 AM
In D5385#46, @jroelofs wrote:

@joerg For the ctype<char> ones, I think that's fine, but I don't think that works for ctype<wchar_t> because classic_table has only at least 256 entries. That would get isascii out of include/__locale, and we can leave the ones in src/locale.cpp, which solves this messiness.

ctype<T>::classic_table() is only defined for ctype<char>. We do perform these same checks in ctype<wchar_t>::do_is() though... I'm not sure what the rules are for classifying wide characters... Shouldn't we just be passing these down to the C library's iswupper() and friends as we do in ctype_byname<wchar_t>::do_is()?

jroelofs updated this revision to Diff 13883.Sep 19 2014, 1:07 PM
jroelofs edited edge metadata.

This time without the isascii part... we can attack that in another patch.

jfb accepted this revision.Sep 19 2014, 1:17 PM
jfb edited edge metadata.

Thanks for splitting. This is all non-contentious as far as I can tell, so I think it's ready to go.

This revision is now accepted and ready to land.Sep 19 2014, 1:17 PM
jroelofs closed this revision.Sep 19 2014, 1:19 PM