For Adding clang check

Message ID 87fti63obg.fsf@oldenburg2.str.redhat.com
State Not applicable
Headers

Commit Message

Florian Weimer Nov. 29, 2019, 11:38 a.m. UTC
  * Jonathan Wakely:

> On 29/11/19 09:25 +0100, Florian Weimer wrote:
>>* kamlesh kumar:
>>
>>> It fixes this.
>>> https://bugs.llvm.org/show_bug.cgi?id=44169
>>
>>What's the rationale for the condition?  What's special about Clang
>>3.5?
>>
>>My understanding is that a compiler needs support for asm aliases
>>*and* the C++ library headers need to be compatible.  Is there a way
>>to determine if libc++ is compatible?
>
> Libc++ already checks for this macro in its <string.h> wrapper:
> https://github.com/llvm/llvm-project/blob/master/libcxx/include/string.h#L62
>
> If the __CORRECT_ISO_CPP_STRING_H_PROTO macro is *not* defined after
> doing #include_next <string.h> (to get the libc header) then libc++
> makes use of a Clang extension to declare new overloads:
> https://github.com/llvm/llvm-project/blob/master/libcxx/include/string.h#L70
>
> The _LIBCPP_PREFERRED_OVERLOAD macro is defined as:
> #    define _LIBCPP_PREFERRED_OVERLOAD __attribute__ ((__enable_if__(true, "")))
>
> That Clang-only attribute means the compiler will use the new
> overloads in preference to the libc strchr. A non-standard extension
> is needed because according to the C++ rules the new overloads should
> be ambiguous with the one that was declared by libc's <string.h>.
>
>
>>For libstdc++ with GCC, the
>>compiler version check covers libstdc++ implicity, but that does not
>>apply to Clang, or libc++ with either compiler.
>
> Libstdc++ with GCC already works.
>
> Libstdc++ with Clang needs this patch.
>
> If I'm reading the libc++ code right ...
>
> Libc++ with GCC should already work, because the __GNUC_PREREQ will
> pass and libc++ is already aware of the existence and effects of the
> __CORRECT_ISO_CPP_STRING_H_PROTO macro. (I doubt libc++ works with
> ancient GCC versions, but if it does they'll get the wrong signatures
> ... well tough luck, use a newer GCC).
>
> Libc++ with Clang doesn't need this patch, because it uses the Clang
> extension, but after this patch it would no longer need to use the
> extension. The right signatures would be declared by glibc.
>
> So of the four combinations, two already work and are unaffected by
> this patch. One already works and is affected, but not in a way users
> will notice (the correct signatures are already there, the patch just
> changes whether they come from glibc or libc++). And one doesn't
> currently work but is fixed by the patch.
>
> I think the patch is right.

Thanks.  I filed: <https://sourceware.org/bugzilla/show_bug.cgi?id=25232>

I tried to distill the discussion into the patch below.

Florian

8<------------------------------------------------------------------8<
From: Kamlesh Kumar <kamleshbhalui@gmail.com>
Subject: <string.h>: Define __CORRECT_ISO_CPP_STRING_H_PROTO for Clang [BZ #25232]

Without the asm redirects, strchr et al. are not const-correct.

libc++ has a wrapper header that works with and without
__CORRECT_ISO_CPP_STRING_H_PROTO (using a Clang extension).  But when
Clang is used with libstdc++ or just C headers, the overloaded functions
with the correct types are not declared.

This change does not impact current GCC (with libstdc++ or libc++).

-----
 string/string.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
  

Comments

Florian Weimer Dec. 5, 2019, 3:51 p.m. UTC | #1
* Florian Weimer:

> * Jonathan Wakely:
>
>> On 29/11/19 09:25 +0100, Florian Weimer wrote:
>>>* kamlesh kumar:
>>>
>>>> It fixes this.
>>>> https://bugs.llvm.org/show_bug.cgi?id=44169
>>>
>>>What's the rationale for the condition?  What's special about Clang
>>>3.5?
>>>
>>>My understanding is that a compiler needs support for asm aliases
>>>*and* the C++ library headers need to be compatible.  Is there a way
>>>to determine if libc++ is compatible?
>>
>> Libc++ already checks for this macro in its <string.h> wrapper:
>> https://github.com/llvm/llvm-project/blob/master/libcxx/include/string.h#L62
>>
>> If the __CORRECT_ISO_CPP_STRING_H_PROTO macro is *not* defined after
>> doing #include_next <string.h> (to get the libc header) then libc++
>> makes use of a Clang extension to declare new overloads:
>> https://github.com/llvm/llvm-project/blob/master/libcxx/include/string.h#L70
>>
>> The _LIBCPP_PREFERRED_OVERLOAD macro is defined as:
>> #    define _LIBCPP_PREFERRED_OVERLOAD __attribute__ ((__enable_if__(true, "")))
>>
>> That Clang-only attribute means the compiler will use the new
>> overloads in preference to the libc strchr. A non-standard extension
>> is needed because according to the C++ rules the new overloads should
>> be ambiguous with the one that was declared by libc's <string.h>.
>>
>>
>>>For libstdc++ with GCC, the
>>>compiler version check covers libstdc++ implicity, but that does not
>>>apply to Clang, or libc++ with either compiler.
>>
>> Libstdc++ with GCC already works.
>>
>> Libstdc++ with Clang needs this patch.
>>
>> If I'm reading the libc++ code right ...
>>
>> Libc++ with GCC should already work, because the __GNUC_PREREQ will
>> pass and libc++ is already aware of the existence and effects of the
>> __CORRECT_ISO_CPP_STRING_H_PROTO macro. (I doubt libc++ works with
>> ancient GCC versions, but if it does they'll get the wrong signatures
>> ... well tough luck, use a newer GCC).
>>
>> Libc++ with Clang doesn't need this patch, because it uses the Clang
>> extension, but after this patch it would no longer need to use the
>> extension. The right signatures would be declared by glibc.
>>
>> So of the four combinations, two already work and are unaffected by
>> this patch. One already works and is affected, but not in a way users
>> will notice (the correct signatures are already there, the patch just
>> changes whether they come from glibc or libc++). And one doesn't
>> currently work but is fixed by the patch.
>>
>> I think the patch is right.
>
> Thanks.  I filed: <https://sourceware.org/bugzilla/show_bug.cgi?id=25232>
>
> I tried to distill the discussion into the patch below.
>
> Florian
>
> 8<------------------------------------------------------------------8<
> From: Kamlesh Kumar <kamleshbhalui@gmail.com>
> Subject: <string.h>: Define __CORRECT_ISO_CPP_STRING_H_PROTO for Clang [BZ #25232]
>
> Without the asm redirects, strchr et al. are not const-correct.
>
> libc++ has a wrapper header that works with and without
> __CORRECT_ISO_CPP_STRING_H_PROTO (using a Clang extension).  But when
> Clang is used with libstdc++ or just C headers, the overloaded functions
> with the correct types are not declared.
>
> This change does not impact current GCC (with libstdc++ or libc++).
>
> -----
>  string/string.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/string/string.h b/string/string.h
> index 73c22a535a..faf997b972 100644
> --- a/string/string.h
> +++ b/string/string.h
> @@ -33,7 +33,8 @@ __BEGIN_DECLS
>  #include <stddef.h>
>  
>  /* Tell the caller that we provide correct C++ prototypes.  */
> -#if defined __cplusplus && __GNUC_PREREQ (4, 4)
> +#if defined __cplusplus && (__GNUC_PREREQ (4, 4) \
> +			    || __glibc_clang_prereq (3, 5))
>  # define __CORRECT_ISO_CPP_STRING_H_PROTO
>  #endif
>  

I have pushed this.  Some libc++ people have been contacted and did not
object.

Thanks,
Florian
  

Patch

diff --git a/string/string.h b/string/string.h
index 73c22a535a..faf997b972 100644
--- a/string/string.h
+++ b/string/string.h
@@ -33,7 +33,8 @@  __BEGIN_DECLS
 #include <stddef.h>
 
 /* Tell the caller that we provide correct C++ prototypes.  */
-#if defined __cplusplus && __GNUC_PREREQ (4, 4)
+#if defined __cplusplus && (__GNUC_PREREQ (4, 4) \
+			    || __glibc_clang_prereq (3, 5))
 # define __CORRECT_ISO_CPP_STRING_H_PROTO
 #endif