This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Ping^4 Define __CORRECT_ISO_CPP_STRING_H_PROTO correctly for Clang.
- From: OndÅej BÃlka <neleai at seznam dot cz>
- To: Brooks Moses <bmoses at google dot com>
- Cc: libc-alpha <libc-alpha at sourceware dot org>, "Joseph S. Myers" <joseph at codesourcery dot com>
- Date: Mon, 23 Dec 2013 18:06:46 +0100
- Subject: Re: [PATCH] Ping^4 Define __CORRECT_ISO_CPP_STRING_H_PROTO correctly for Clang.
- Authentication-results: sourceware.org; auth=none
- References: <CAOxa4KoXaQPUSZuniRVhUfcDpaW+pxM5JZ4qtHBjdRu4Lgjzxw at mail dot gmail dot com>
Joseph, are you ok with this? This is simple addition that could go to 2.19,
is Brooks rationale sufficient for you?
On Thu, Dec 19, 2013 at 10:12:46AM -0800, Brooks Moses wrote:
> Ping^4?
>
> On Wed, Dec 11, 2013 at 6:02 PM, Brooks Moses <bmoses@google.com> wrote:
> > Ping^3.
> >
> > In the string/string.h and string/strings.h headers, we have a couple
> > of macros that "tell the caller that we provide correct C++
> > prototypes" according to the comment; they are used to determine
> > whether to wrap some prototypes in "extern "C++"" (and provide
> > multiple overloads of them, and some other magic) when __cplusplus is
> > defined.
> >
> > The macros are set to check for sufficiently-recent GCC versions (4.4
> > and later), but this is not the right check for non-GCC compilers. In
> > particular, these macros should also be set when using Clang -- if
> > they are not set, then Clang will be unable to correctly diagnose a
> > number of subtle bugs that will be errors in GCC compilations.
> >
> > As per discussion on earlier versions of this patch, rather than
> > restrict the fix to Clang per se, we assume that all C++ compilers that
> > claim to fully support C++98 are using a standard-conforming C++
> > standard library, which seems pretty reasonable. Clang has been
> > providing an appropriate value of __cplusplus since May 2012.
> >
> >
> > As it's been a while since my last ping, here's the previous discussion
> > of why this is the best approach:
> >
> > On Mon, Sep 9, 2013 at 3:00 PM, Brooks Moses <bmoses@google.com> wrote:
> >> On Thu, Aug 29, 2013 at 5:18 AM, Joseph S. Myers
> >> <joseph@codesourcery.com> wrote:
> >>> What's the logic behind the GCC version check in the first place? Is it
> >>> about compiler (language) features, or about corresponding support in the
> >>> standard C++ library? If the former, do you need a Clang version check?
> >>> If the latter, do you need a check on what version of libstdc++ or libc++
> >>> might be used with Clang?
> >>
> >> This appears to be about corresponding support in libstdc++. The
> >> original patch was actually only posted to gcc-patches@, here [1]:
> >> http://gcc.gnu.org/ml/gcc-patches/2009-01/msg01457.html
> >> Prior to the patch, libstdc++'s cstring provided the C++ declarations
> >> of the string.h functions. After the patch, the libstdc++ cstring
> >> only provides the C++ string.h declarations if these macros are not
> >> defined, so as to avoid conflicting with glibc's string.h
> >> declarations.
> >>
> >> In libc++, the corresponding declarations are guarded by a simple
> >> "!defined(__GLIBC__)" check, and have been since the very early days
> >> of the project; thus this should be safe for that as well [2].
> >>
> >> In principle, the correct solution might be replacing the GCC version
> >> check with a check on __GLIBCXX__ and _LIBCPP_VERSION. However,
> >> __GLIBCXX__ is not monotonic with the GCC version[3] so this would be
> >> a very messy check, and there's the issue of getting the __GLIBCXX__
> >> value in a way that is guaranteed not to introduce circular header
> >> dependencies in libstdc++ or any other C++ library. So unfortunately
> >> I don't think that's workable in practice.
> >>
> >> I brought this up with the Clang development mailing list, and the
> >> answer I got there was that "In practice, any of the solutions is
> >> probably fine: glibc gets upgraded along with the system, which
> >> implies a newer gcc, which implies clang will find a newer libstdc++"
> >> [4].
> >>
> >> Thus, I'd like to go with Andrew Pinski's idea of using a
> >> "__cplusplus>=199711L" check, and use the legacy behavior (check for
> >> GCC of 4.4 or later) otherwise. As I explain in the code comment,
> >> this basically assumes that C++ compilers that claim to fully support
> >> C++98 are using a standard-conforming C++ standard library, which
> >> seems pretty reasonable. Clang has been providing an appropriate
> >> value of __cplusplus since May of last year.
> >>
> >> The revised patch is attached. I've tested it by compiling a file
> >> that includes the headers and checks the macro values, with GCC 4.6,
> >> GCC 4.8, and a recent build of Clang, and I confirmed that the macros
> >> are defined as expected in all cases. I've attached the test program
> >> as well.
> >>
> >> Is this version ok to commit?
> >>
> >> Thanks,
> >> - Brooks
> >>
> >>
> >> [1] For archival purposes: There was also a bugfix on it sent to libc-hacker@:
> >> http://sourceware.org/ml/libc-hacker/2009-01/msg00013.html
> >>
> >> [2]: Discussion of the libc++ cstring prototypes here:
> >> http://llvm.org/svn/llvm-project/libcxx/trunk/include/cstring
> >> http://llvm.org/bugs/show_bug.cgi?id=7983
> >>
> >> [3]: See list of __GLIBCXX__ values over the years:
> >> http://gcc.gnu.org/onlinedocs/libstdc++/manual/abi.html
> >>
> >> [4] This quote is from email from Eli Friedman:
> >> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2013-August/031657.html
> >>
> >> [5] Clang generally followed GCC practice for __cplusplus values;
> >> discussion and patch here:
> >> http://clang-developers.42468.n3.nabble.com/Value-of-cplusplus-in-GNU-modes-td3957527.html
> >> http://llvm.org/viewvc/llvm-project?revision=156113&view=revision
> >
> >
> > ---
> > 2013-09-09 Brooks Moses <bmoses@google.com>
> >
> > * string/string.h (__CORRECT_ISO_CPP_STRING_H_PROTO): Define for
> > all compilers that claim C++98 compliance, not just GCC.
> > * string/strings.h (__CORRECT_ISO_CPP_STRINGS_H_PROTO):
> > Likewise.
> >
> > string/string.h | 8 ++++++--
> > string/strings.h | 8 ++++++--
> > 2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/string/string.h b/string/string.h
> > index ecc3fef..3e66674 100644
> > --- a/string/string.h
> > +++ b/string/string.h
> > @@ -31,8 +31,12 @@ __BEGIN_DECLS
> > #define __need_NULL
> > #include <stddef.h>
> >
> > -/* Tell the caller that we provide correct C++ prototypes. */
> > -#if defined __cplusplus && __GNUC_PREREQ (4, 4)
> > +/* Provide correct C++ prototypes, and indicate this to the caller. This
> > + requires a compatible C++ standard library. As a heuristic, we provide
> > + these when the compiler indicates full conformance with C++98 or later,
> > + and for older GCC versions that are known to provide a compatible
> > + libstdc++. */
> > +#if __cplusplus >= 199711L || (defined __cplusplus && __GNUC_PREREQ (4, 4))
> > # define __CORRECT_ISO_CPP_STRING_H_PROTO
> > #endif
> >
> > diff --git a/string/strings.h b/string/strings.h
> > index dad0a04..190147b 100644
> > --- a/string/strings.h
> > +++ b/string/strings.h
> > @@ -27,8 +27,12 @@
> > # define __need_size_t
> > # include <stddef.h>
> >
> > -/* Tell the caller that we provide correct C++ prototypes. */
> > -# if defined __cplusplus && __GNUC_PREREQ (4, 4)
> > +/* Provide correct C++ prototypes, and indicate this to the caller. This
> > + requires a compatible C++ standard library. As a heuristic, we provide
> > + these when the compiler indicates full conformance with C++98 or later,
> > + and for older GCC versions that are known to provide a compatible
> > + libstdc++. */
> > +# if __cplusplus >= 199711L || (defined __cplusplus && __GNUC_PREREQ (4, 4))
> > # define __CORRECT_ISO_CPP_STRINGS_H_PROTO
> > # endif
> >
> > --
> > 1.8.5.1
> >