Re: [PATCH] Ping^4 Define __CORRECT_ISO_CPP_STRING_H_PROTO correctly for Clang.

[Brooks Moses <bmoses at google dot com>]

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
>