Message ID | AM5PR0802MB26101582FEA5EBF10B7CE8F583BE0@AM5PR0802MB2610.eurprd08.prod.outlook.com |
---|---|
State | Superseded |
Headers |
Received: (qmail 71906 invoked by alias); 16 Nov 2016 18:59:51 -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 71893 invoked by uid 89); 16 Nov 2016 18:59:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:1119 X-HELO: EUR01-DB5-obe.outbound.protection.outlook.com From: Wilco Dijkstra <Wilco.Dijkstra@arm.com> To: "libc-alpha@sourceware.org" <libc-alpha@sourceware.org> CC: nd <nd@arm.com> Subject: [PATCH] Do not transform strchr into rawmemchr Date: Wed, 16 Nov 2016 18:59:36 +0000 Message-ID: <AM5PR0802MB26101582FEA5EBF10B7CE8F583BE0@AM5PR0802MB2610.eurprd08.prod.outlook.com> References: <AM5PR0802MB26102CD0C9B41C82C3CF58A483BE0@AM5PR0802MB2610.eurprd08.prod.outlook.com> In-Reply-To: <AM5PR0802MB26102CD0C9B41C82C3CF58A483BE0@AM5PR0802MB2610.eurprd08.prod.outlook.com> authentication-results: spf=none (sender IP is ) smtp.mailfrom=Wilco.Dijkstra@arm.com; x-microsoft-exchange-diagnostics: 1; AM5PR0802MB2611; 7:SiqPqNBHpwpiLVWURCfN7TGh5fS652mskQIVBXMfyE3xlA3LdMSjmkB63DBuTN94HMf60/O39y+yT23qyUouMbFftqMsFA+RaozRTwRUWzyccEjnFphMk6s97iUD9bj8RMLibFEe3MFw2kDXxk2dsPtI5/oYe4e9DPzH7ixVOJXDlstGBgdB0/6nY8dcaoFZm3BI18ose53QnhC3OzAADEmDre4tHitcyVepTKG7LlKgjVR268reMEcsX/saq2EVc5a9hIn0eVLaKX+p/TGs6/30XuVm97lR8IftxIqrorOMkh1F1O5V/f9/AlP+1R1WIRmP3fYSMjcJZznEKQ/YRNNMajHdTVtQDroOlllFrSs= x-ms-office365-filtering-correlation-id: f1aab429-670c-4963-5b2d-08d40e52b5f6 x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(22001); SRVR:AM5PR0802MB2611; nodisclaimer: True x-microsoft-antispam-prvs: <AM5PR0802MB261166029D6FE31AB623E2CF83BE0@AM5PR0802MB2611.eurprd08.prod.outlook.com> x-exchange-antispam-report-test: UriScan:(180628864354917); x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6060326)(6040281)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6055026)(6061324)(6041223); SRVR:AM5PR0802MB2611; BCL:0; PCL:0; RULEID:; SRVR:AM5PR0802MB2611; x-forefront-prvs: 01283822F8 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(7916002)(189002)(54534003)(377424004)(199003)(8936002)(81166006)(81156014)(575784001)(77096005)(2351001)(2501003)(450100001)(4326007)(6506003)(189998001)(86362001)(5660300001)(2906002)(5640700001)(2950100002)(76576001)(122556002)(3660700001)(68736007)(87936001)(50986999)(8676002)(101416001)(110136003)(7696004)(3846002)(92566002)(76176999)(6916009)(2900100001)(3280700002)(54356999)(4001150100001)(106356001)(102836003)(33656002)(66066001)(105586002)(106116001)(7736002)(7846002)(305945005)(6116002)(74316002)(97736004)(9686002)(40753002); DIR:OUT; SFP:1101; SCL:1; SRVR:AM5PR0802MB2611; H:AM5PR0802MB2610.eurprd08.prod.outlook.com; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; received-spf: None (protection.outlook.com: arm.com does not designate permitted sender hosts) spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Nov 2016 18:59:36.6298 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM5PR0802MB2611 |
Commit Message
Wilco Dijkstra
Nov. 16, 2016, 6:59 p.m. UTC
GLIBC uses strchr (s, '\0') as an idiom to find the end of a string. This is transformed into rawmemchr by the bits/string2.h header. However this is generally slower than strlen on most targets, even when an optimized rawmemchr implementation exists. Since GCC7 optimizes strchr (s, '\0') to strlen (s) + s, the GLIBC headers should not transform this to rawmemchr. GLIBC tests pass, OK for commit? ChangeLog: 2015-11-16 Wilco Dijkstra <wdijkstr@arm.com> * string/bits/string2.h (strchr): Use __builtin_strchr. --
Comments
On Wed, Nov 16, 2016 at 1:59 PM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote: > GLIBC uses strchr (s, '\0') as an idiom to find the end of a string. > This is transformed into rawmemchr by the bits/string2.h header. > However this is generally slower than strlen on most targets, even when > an optimized rawmemchr implementation exists. Since GCC7 optimizes > strchr (s, '\0') to strlen (s) + s, the GLIBC headers should not > transform this to rawmemchr. I endorse the removal of the non-optimization, but ... > #ifndef _HAVE_STRING_ARCH_strchr > -extern void *__rawmemchr (const void *__s, int __c); > -# define strchr(s, c) \ > - (__extension__ (__builtin_constant_p (c) && !__builtin_constant_p (s) \ > - && (c) == '\0' \ > - ? (char *) __rawmemchr (s, c) \ > - : __builtin_strchr (s, c))) > +# define strchr(s, c) __builtin_strchr (s, c) > #endif ... wouldn't it be just as effective to remove this block entirely? That is, don't #define strchr at all. zw
Zack Weinberg wrote: > I endorse the removal of the non-optimization, but ... > ... wouldn't it be just as effective to remove this block entirely? > That is, don't #define strchr at all. Well that's a good question. Most string functions directly use the GCC builtin either via a define or via an inline function, so I simply follow that convention. However is there any benefit in doing so? If there is no benefit then we could remove a lot of code from the GLIBC headers, particularly string.h (and your covariance patch could become even simpler without the inline functions). Wilco
On 11/17/2016 06:22 AM, Wilco Dijkstra wrote: > Zack Weinberg wrote: > >> I endorse the removal of the non-optimization, but ... > >> ... wouldn't it be just as effective to remove this block entirely? >> That is, don't #define strchr at all. > > Well that's a good question. Most string functions directly use the GCC > builtin either via a define or via an inline function, so I simply follow that > convention. > > However is there any benefit in doing so? If there is no benefit then we could > remove a lot of code from the GLIBC headers, particularly string.h (and your > covariance patch could become even simpler without the inline functions). Funny you should mention that; I'm right now working on a branch that completely removes both bits/string.h and bits/string2.h -- not because I want to actually do that (though I'm hoping we can do _most_ of it), but because I think it will be handy to have around for experiments to find out whether the existing string inlines are actually any use. Regarding __builtin_strfoo versus strfoo, I _think_ an inline forward to __builtin_strfoo does something useful when either the name or the type signature of 'strfoo' doesn't match what the compiler expects. So, for instance, we will want them in the covariance case as long as the C++ compiler hasn't had its builtins updated to know that there are now two overloads of strwhatever -- but then they become unnecessary. On the other hand, in the strchr/rawmemchr case, the basic prototype for strchr should be enough to clue the compiler that it knows what this function does, with no further prompting. I could be wrong about this. cc: Joseph: do you know whether my understanding is accurate? zw
On Thu, 17 Nov 2016, Zack Weinberg wrote: > Regarding __builtin_strfoo versus strfoo, I _think_ an inline forward to > __builtin_strfoo does something useful when either the name or the type > signature of 'strfoo' doesn't match what the compiler expects. So, for > instance, we will want them in the covariance case as long as the C++ > compiler hasn't had its builtins updated to know that there are now two > overloads of strwhatever -- but then they become unnecessary. On the > other hand, in the strchr/rawmemchr case, the basic prototype for strchr > should be enough to clue the compiler that it knows what this function > does, with no further prompting. I could be wrong about this. > > cc: Joseph: do you know whether my understanding is accurate? Mapping strfoo to __builtin_strfoo may also be useful in the case where strfoo is not a C90 function (so the user may have used -std=c90 -D_GNU_SOURCE, getting the declaration of strfoo but without it being built-in in the compiler). In the strchr case, it's a C90 function so aside from inlines for C++ overloads mapping to __builtin_strchr should not be of use (if someone uses -fno-builtin or -ffreestanding there should be no expectation that headers need to try to optimize things anyway, and even the case where -std with feature test macros means the function is declared but not built-in is pretty marginal).
Joseph Myers wrote: > On Thu, 17 Nov 2016, Zack Weinberg wrote: > > Regarding __builtin_strfoo versus strfoo, I _think_ an inline forward to > > __builtin_strfoo does something useful when either the name or the type > > signature of 'strfoo' doesn't match what the compiler expects. So, for > > instance, we will want them in the covariance case as long as the C++ > > compiler hasn't had its builtins updated to know that there are now two > > overloads of strwhatever -- but then they become unnecessary. On the > > other hand, in the strchr/rawmemchr case, the basic prototype for strchr > > should be enough to clue the compiler that it knows what this function > > does, with no further prompting. I could be wrong about this. > Mapping strfoo to __builtin_strfoo may also be useful in the case where > strfoo is not a C90 function (so the user may have used -std=c90 > -D_GNU_SOURCE, getting the declaration of strfoo but without it being > built-in in the compiler). Also I believe this remapping is needed for non-C90 functions that are used inside GLIBC itself. Eg. stpcpy is used a lot, and so is mapped to __stpcpy. Because __stpcpy is not a GCC builtin, it is mapped to __builtin_stpcpy. Oddly enough all this happens in string/bits/string2.h, which is only included for some optimization settings. If GLIBC is for example built for -Os it won't do this remapping, which means the namespace is polluted with non-C90 symbols. So should these internal remappings be moved to string.h and made conditional, so we only do this when building GLIBC? > In the strchr case, it's a C90 function so aside from inlines for C++ > overloads mapping to __builtin_strchr should not be of use (if someone > uses -fno-builtin or -ffreestanding there should be no expectation that > headers need to try to optimize things anyway, and even the case where > -std with feature test macros means the function is declared but not > built-in is pretty marginal). I'll remove the strchr, strncpy, strcspn, strspn and strpbrk cases from string2.h then. Wilco
On Fri, 18 Nov 2016, Wilco Dijkstra wrote: > Also I believe this remapping is needed for non-C90 functions that are used > inside GLIBC itself. Eg. stpcpy is used a lot, and so is mapped to __stpcpy. > Because __stpcpy is not a GCC builtin, it is mapped to __builtin_stpcpy. > > Oddly enough all this happens in string/bits/string2.h, which is only included > for some optimization settings. If GLIBC is for example built for -Os it won't do > this remapping, which means the namespace is polluted with non-C90 symbols. > So should these internal remappings be moved to string.h and made conditional, > so we only do this when building GLIBC? (Bug 19463 is linknamespace failures with -Os and bug 15105 is localplt failures, but even if stpcpy appears in one or both sets of failures, there are a lot more as well, so fixing things for stpcpy might be relevant to such bugs but wouldn't fix them.) Since the built-in expansions of stpcpy may not be expanded inline, we also have libc_hidden_builtin_proto (stpcpy) #if (!IS_IN (libc) || !defined SHARED) \ && !defined NO_MEMPCPY_STPCPY_REDIRECT /* Redirect calls to __builtin_mempcpy and __builtin_stpcpy to call __mempcpy and __stpcpy if not inlined. */ extern __typeof (mempcpy) mempcpy __asm__ ("__mempcpy"); extern __typeof (stpcpy) stpcpy __asm__ ("__stpcpy"); #endif in include/string.h - the first to cover the case of references to stpcpy inside shared libc, the rest to cover references elsewhere. Thus, it should be possible just to call stpcpy directly everywhere in glibc rather than __stpcpy, and rely on those redirections to ensure that the calls actually end up being namespace-clean (but can be inlined if appropriate as GCC knows about stpcpy as a built-in function in gnu11 mode). Then the definition of __stpcpy in bits/string2.h should not be needed for building glibc.
diff --git a/string/bits/string2.h b/string/bits/string2.h index 80987602f34ded483854bcea86dabd5b81e42a18..f0e2ce9cd87033698236e7878c63a2e5f9bb1887 100644 --- a/string/bits/string2.h +++ b/string/bits/string2.h @@ -59,12 +59,7 @@ Â Â Â #ifndef _HAVE_STRING_ARCH_strchr -extern void *__rawmemchr (const void *__s, int __c); -#Â define strchr(s, c) \ -Â (__extension__ (__builtin_constant_p (c) && !__builtin_constant_p (s)Â Â Â Â Â Â Â Â Â Â Â Â Â \ -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â && (c) == '\0'Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \ -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ? (char *) __rawmemchr (s, c)Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \ -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â : __builtin_strchr (s, c))) +# define strchr(s, c) __builtin_strchr (s, c) Â #endif