This is an archive of the discontinued LLVM Phabricator instance.

Implement [depr.c.headers]
AbandonedPublic

Authored by rsmith on Sep 9 2015, 6:24 PM.

Details

Summary

This patch provides the <foo.h> headers required by [depr.c.headers]. Previously, libc++ would rely on the underlying C library to provide these files, but it provides the C versions of the headers which typically miss some overloads required for a C++ standard library, and define macros that the C++ headers are not permitted to define.

I also removed some code which did this:

#ifdef isxdigit
inline _LIBCPP_INLINE_VISIBILITY int __libcpp_isxdigit(int __c) {return isxdigit(__c);}
#undef isxdigit
inline _LIBCPP_INLINE_VISIBILITY int isxdigit(int __c) {return __libcpp_isxdigit(__c);}
#endif  // isxdigit

This doesn't work when defining the function in the global namespace (there already is a function named isxdigit in that scope), and is not necessary for correctness -- the C standard requires that these symbols are defined as functions, even if there's a macro present. So instead we just use this:

#undef isxdigit

This theoretically produces slower code, in that the macro may do something that's faster than a function call. However, in at least glibc, the libc function is defined as an inline function anyway (when building at -O1 or above), so there is no actual difference.

Diff Detail

Repository
rL LLVM

Event Timeline

rsmith updated this revision to Diff 34397.Sep 9 2015, 6:24 PM
rsmith retitled this revision from to Implement [depr.c.headers].
rsmith updated this object.
rsmith added reviewers: mclow.lists, EricWF, chandlerc.
rsmith set the repository for this revision to rL LLVM.
rsmith added a subscriber: cfe-commits.

Each of the foo.h files added here was svn cp'd from the corresponding cfoo file. The listed diffs are against the base file. Likewise, __nullptr was copied from cstddef.

(Please disregard the changes to lib/buildit.)

mclow.lists edited edge metadata.Sep 14 2015, 7:07 AM

I have two concerns about this patch (w/o commenting on the actual code).

  1. Until very recently, I was under the impression that C libraries _either_ defined a macro, or had a function. I was quite surprised to find that glibc did both. Have you checked other C libraries (Apple, FreeBSD, Android, Windows) to see if they also define both? The purpose of this code:
#ifdef getc
inline _LIBCPP_INLINE_VISIBILITY int __libcpp_getc(FILE* __stream) {return getc(__stream);}
#undef getc
inline _LIBCPP_INLINE_VISIBILITY int getc(FILE* __stream) {return __libcpp_getc(__stream);}
#endif  // getc

is to (a) remove the macro getc, since we are required to define std::getc and (b) to provide a global function with the same functionality that we can hoist into namespace std.

  1. This adds a lot of header files. Each header file slows down compilation, and standard library header files get included *a lot*. We may not be able to avoid this, but we should think about the costs here.
EricWF edited edge metadata.Oct 5 2015, 7:10 PM

I think thing change will help us close a number out outstanding bugs. I don't have any fundamental objections to this approach. However the size of this patch scares me. I understand the changes are mostly mechanical but their size can hide things. For example has anybody noticed that your internal changes to <deque> are in this patch? It would be nice to break this down into more digestible pieces that could be quickly spot checked.

rsmith added a subscriber: rsmith.Oct 6 2015, 2:58 PM
rsmith added a comment.Oct 7 2015, 2:38 PM

Marshall: ping, does the below satisfy your concerns about the direction
here?

rsmith abandoned this revision.Oct 14 2015, 2:41 PM

Patch has been split up and the individual parts have all been committed (except the module map changes, which are currently problematic due to libc / libc++ layering issues).