[PATCH] For Adding clang check

[PATCH] For Adding clang check

[Kamlesh Kumar]

ChangeLog :

2019-11-28  Kamlesh Kumar  <kamleshbhalui@gmail.com>

       * string/string.h (__glibc_clang_prereq): Used.
---
 string/string.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/string/string.h b/string/string.h
index 73c22a535a..06a8977165 100644
--- a/string/string.h
+++ b/string/string.h
@@ -33,7 +33,7 @@ __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
 
-- 
2.17.1

Re: [PATCH] For Adding clang check

[Florian Weimer]

* Kamlesh Kumar:

> ChangeLog :
>
> 2019-11-28  Kamlesh Kumar  <kamleshbhalui@gmail.com>
>
>        * string/string.h (__glibc_clang_prereq): Used.
> ---
>  string/string.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/string/string.h b/string/string.h
> index 73c22a535a..06a8977165 100644
> --- a/string/string.h
> +++ b/string/string.h
> @@ -33,7 +33,7 @@ __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

Sorry, what's the purpose of this change?  Thanks.

If it fixes a user-visible problem, it should reference a bug in
Bugzilla.

Re: [PATCH] For Adding clang check

[kamlesh kumar]

It fixes this.
https://bugs.llvm.org/show_bug.cgi?id=44169


On Fri, Nov 29, 2019 at 12:22 AM Florian Weimer <fw@deneb.enyo.de> wrote:
>
> * Kamlesh Kumar:
>
> > ChangeLog :
> >
> > 2019-11-28  Kamlesh Kumar  <kamleshbhalui@gmail.com>
> >
> >        * string/string.h (__glibc_clang_prereq): Used.
> > ---
> >  string/string.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/string/string.h b/string/string.h
> > index 73c22a535a..06a8977165 100644
> > --- a/string/string.h
> > +++ b/string/string.h
> > @@ -33,7 +33,7 @@ __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
>
> Sorry, what's the purpose of this change?  Thanks.
>
> If it fixes a user-visible problem, it should reference a bug in
> Bugzilla.

Re: [PATCH] For Adding clang check

[Florian Weimer]

* 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?  For libstdc++ with GCC, the
compiler version check covers libstdc++ implicity, but that does not
apply to Clang, or libc++ with either compiler.

> On Fri, Nov 29, 2019 at 12:22 AM Florian Weimer <fw@deneb.enyo.de> wrote:
>>
>> * Kamlesh Kumar:
>>
>> > ChangeLog :
>> >
>> > 2019-11-28  Kamlesh Kumar  <kamleshbhalui@gmail.com>
>> >
>> >        * string/string.h (__glibc_clang_prereq): Used.
>> > ---
>> >  string/string.h | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/string/string.h b/string/string.h
>> > index 73c22a535a..06a8977165 100644
>> > --- a/string/string.h
>> > +++ b/string/string.h
>> > @@ -33,7 +33,7 @@ __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
>>
>> Sorry, what's the purpose of this change?  Thanks.
>>
>> If it fixes a user-visible problem, it should reference a bug in
>> Bugzilla.

Re: [PATCH] For Adding clang check

[kamlesh kumar]

Hi Florian,
While compiling this (https://godbolt.org/z/uV2Pjp) c++ program with
clang (it uses libstdc++ by default)
static_assert fails.
that is because
this macro  __CORRECT_ISO_CPP_STRING_H_PROTO does not get defined when
clang is used, because it is like this in string.h

#if defined __cplusplus && __GNUC_PREREQ (4, 4)
# define __CORRECT_ISO_CPP_STRING_H_PROTO
#endif


And return type of strchr  in this case is char *, while we expect it
to be const char *.
That does not happen because latter declration is hidden by this macro
__CORRECT_ISO_CPP_STRING_H_PROTO.

I have used clang 3.5  here in check, because this is minimum version
which worked with libc++ and libstdc++.

,kamlesh

On Fri, Nov 29, 2019 at 1:55 PM Florian Weimer <fw@deneb.enyo.de> 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?  For libstdc++ with GCC, the
> compiler version check covers libstdc++ implicity, but that does not
> apply to Clang, or libc++ with either compiler.
>
> > On Fri, Nov 29, 2019 at 12:22 AM Florian Weimer <fw@deneb.enyo.de> wrote:
> >>
> >> * Kamlesh Kumar:
> >>
> >> > ChangeLog :
> >> >
> >> > 2019-11-28  Kamlesh Kumar  <kamleshbhalui@gmail.com>
> >> >
> >> >        * string/string.h (__glibc_clang_prereq): Used.
> >> > ---
> >> >  string/string.h | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/string/string.h b/string/string.h
> >> > index 73c22a535a..06a8977165 100644
> >> > --- a/string/string.h
> >> > +++ b/string/string.h
> >> > @@ -33,7 +33,7 @@ __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
> >>
> >> Sorry, what's the purpose of this change?  Thanks.
> >>
> >> If it fixes a user-visible problem, it should reference a bug in
> >> Bugzilla.

Re: [PATCH] For Adding clang check

[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.

Re: [PATCH] For Adding clang check

[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
 

Re: [PATCH] For Adding clang check

[Florian Weimer]

* 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