Message ID | 87fti63obg.fsf@oldenburg2.str.redhat.com |
---|---|
State | Not applicable |
Headers |
Received: (qmail 44584 invoked by alias); 29 Nov 2019 11:38:23 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 44568 invoked by uid 89); 29 Nov 2019 11:38:23 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.5 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: us-smtp-delivery-1.mimecast.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575027499; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=7rZ1AFY+oJCSGHYdnjMU4/nOIYUXOkZOCNa71vB4QFU=; b=NzMEfAVd/kGFg5NbJBP1LvUqCWruPCikm/SUZmY40i9+FG5SQVtfNqrVKH5iqBBL+/oK3+ ilfUEDQlONa8Eo5gfdZZP9BGBYh6dFulvDdWiddedEQljW+yYH7ksJubRWzTssftyrIXVz Yjczf2rUYJTaMqbT90bk4O6afVDS3Mk= From: Florian Weimer <fweimer@redhat.com> To: Jonathan Wakely <jwakely@redhat.com> Cc: kamlesh kumar <kamleshbhalui@gmail.com>, libc-alpha@sourceware.org, Andreas Schwab <schwab@suse.de>, Brooks Moses <bmoses@google.com> Subject: Re: [PATCH] For Adding clang check References: <20191128184733.27377-1-kamleshbhalui@gmail.com> <87tv6n7s0t.fsf@mid.deneb.enyo.de> <CABKRkgj80BU+yuJRrvb0WTPYexKT4NDBj38dwTZqc_Xm27v+_Q@mail.gmail.com> <87a78fay23.fsf@mid.deneb.enyo.de> <20191129105759.GX11522@redhat.com> Date: Fri, 29 Nov 2019 12:38:11 +0100 In-Reply-To: <20191129105759.GX11522@redhat.com> (Jonathan Wakely's message of "Fri, 29 Nov 2019 10:57:59 +0000") Message-ID: <87fti63obg.fsf@oldenburg2.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 Content-Type: text/plain Content-Transfer-Encoding: quoted-printable |
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: > * 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
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