Use correct includes in benchtests
Commit Message
Currently the benchtests are run with internal GLIBC headers, which is incorrect.
Defining _ISOMAC in the makefile ensures the internal headers are bypassed.
Fix all tests which were relying on internal defines or includes.
Benchtests now run with correct headers on AArch64.
OK for commit?
ChangeLog:
2018-03-15 Wilco Dijkstra <wdijkstr@arm.com>
* benchtests/Makefile: Define _ISOMAC.
* benchtests/bench-strcoll.c: Add missing sys/stat.h include.
* benchtests/bench-string.h: Define inhibit_loop_to_libcall macro.
* benchtests/bench-strstr.c: Define empty libc_hidden_builtin_def.
* benchtests/bench-strtok.c (oldstrtok): Use rawmemchr.
--
Comments
On Thursday 15 March 2018 07:59 PM, Wilco Dijkstra wrote:
> Currently the benchtests are run with internal GLIBC headers, which is incorrect.
> Defining _ISOMAC in the makefile ensures the internal headers are bypassed.
> Fix all tests which were relying on internal defines or includes.
>
> Benchtests now run with correct headers on AArch64.
>
> OK for commit?
>
> ChangeLog:
> 2018-03-15 Wilco Dijkstra <wdijkstr@arm.com>
>
> * benchtests/Makefile: Define _ISOMAC.
> * benchtests/bench-strcoll.c: Add missing sys/stat.h include.
> * benchtests/bench-string.h: Define inhibit_loop_to_libcall macro.
> * benchtests/bench-strstr.c: Define empty libc_hidden_builtin_def.
> * benchtests/bench-strtok.c (oldstrtok): Use rawmemchr.
>
OK.
Siddhesh
On Thu, Mar 15, 2018 at 10:29 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> Currently the benchtests are run with internal GLIBC headers, which is incorrect.
> Defining _ISOMAC in the makefile ensures the internal headers are bypassed.
> Fix all tests which were relying on internal defines or includes.
...
> -CPPFLAGS-nonlib += -DDURATION=$(BENCH_DURATION)
> +CPPFLAGS-nonlib += -DDURATION=$(BENCH_DURATION) -D_ISOMAC
Is there a reason you can't change `lib := nonlib` to `lib :=
testsuite` in the invocation of libof-iterator.mk, instead? Several
other places in the Makefile would need to be changed, but using the
testsuite module is abstractly more appropriate and might have other
desirable effects in the future (e.g. not including libc-symbols.h at
all).
zw
On Thursday 15 March 2018 08:12 PM, Zack Weinberg wrote:
> On Thu, Mar 15, 2018 at 10:29 AM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>> Currently the benchtests are run with internal GLIBC headers, which is incorrect.
>> Defining _ISOMAC in the makefile ensures the internal headers are bypassed.
>> Fix all tests which were relying on internal defines or includes.
> ...
>> -CPPFLAGS-nonlib += -DDURATION=$(BENCH_DURATION)
>> +CPPFLAGS-nonlib += -DDURATION=$(BENCH_DURATION) -D_ISOMAC
>
> Is there a reason you can't change `lib := nonlib` to `lib :=
> testsuite` in the invocation of libof-iterator.mk, instead? Several
> other places in the Makefile would need to be changed, but using the
> testsuite module is abstractly more appropriate and might have other
> desirable effects in the future (e.g. not including libc-symbols.h at
> all).
It's been a while since I touched those bits but IIRC nonlib is treated
specially, which is why I had made it nonlib. If it's not, then calling
it benchtests is a good idea (and then have testsuite for all the test
programs, although I believe some test programs test internals; they
need to be split out too eventually), but orthogonal to this change.
Siddhesh
On 03/15/2018 03:29 PM, Wilco Dijkstra wrote:
> 2018-03-15 Wilco Dijkstra<wdijkstr@arm.com>
>
> * benchtests/Makefile: Define _ISOMAC.
> * benchtests/bench-strcoll.c: Add missing sys/stat.h include.
> * benchtests/bench-string.h: Define inhibit_loop_to_libcall macro.
> * benchtests/bench-strstr.c: Define empty libc_hidden_builtin_def.
> * benchtests/bench-strtok.c (oldstrtok): Use rawmemchr.
I think this broken --enable-static-pie builds for some reason:
In file included from bench-timing-type.c:19:0:
bench-timing.h:19:0: error: "attribute_hidden" redefined [-Werror]
#define attribute_hidden
In file included from <command-line>:0:0:
./../include/libc-symbols.h:361:0: note: this is the location of the
previous definition
# define attribute_hidden __attribute__ ((visibility ("hidden")))
Any suggestions how to fix this? Just stick an #undef attribute_hidden
in front of it?
Thanks,
Florian
Florian Weimer wrote:
> I think this broken --enable-static-pie builds for some reason:
>
> In file included from bench-timing-type.c:19:0:
> bench-timing.h:19:0: error: "attribute_hidden" redefined [-Werror]
> #define attribute_hidden
> In file included from <command-line>:0:0:
> ./../include/libc-symbols.h:361:0: note: this is the location of the
> previous definition
> # define attribute_hidden __attribute__ ((visibility ("hidden")))
>
> Any suggestions how to fix this? Just stick an #undef attribute_hidden
> in front of it?
Hmm that looks like a preincluded file, which is done just before we
define _ISOMAC... So I wonder whether it would be better to fix
Makeconfig to pre-includes after the CPPFLAGS rather than in the
middle of it?
CPPFLAGS = $(config-extra-cppflags) $(CPPUNDEFS) $(CPPFLAGS-config) \
$($(subdir)-CPPFLAGS) \
$(+includes) $(defines) $(module-cppflags) \
-include $(..)include/libc-symbols.h $(sysdep-CPPFLAGS) \
$(CPPFLAGS-$(suffix $@)) \
$(foreach lib,$(libof-$(basename $(@F))) \
$(libof-$(<F)) $(libof-$(@F)),$(CPPFLAGS-$(lib))) \
Wilco
On 03/16/2018 07:06 PM, Wilco Dijkstra wrote:
> Florian Weimer wrote:
>
>> I think this broken --enable-static-pie builds for some reason:
>>
>> In file included from bench-timing-type.c:19:0:
>> bench-timing.h:19:0: error: "attribute_hidden" redefined [-Werror]
>> #define attribute_hidden
>> In file included from <command-line>:0:0:
>> ./../include/libc-symbols.h:361:0: note: this is the location of the
>> previous definition
>> # define attribute_hidden __attribute__ ((visibility ("hidden")))
>>
>> Any suggestions how to fix this? Just stick an #undef attribute_hidden
>> in front of it?
>
> Hmm that looks like a preincluded file, which is done just before we
> define _ISOMAC... So I wonder whether it would be better to fix
> Makeconfig to pre-includes after the CPPFLAGS rather than in the
> middle of it?
I think Zack's suggestion regarding the change of module will help here.
Thanks,
Florian
On Friday 16 March 2018 11:42 PM, Florian Weimer wrote:
> On 03/16/2018 07:06 PM, Wilco Dijkstra wrote:
>> Florian Weimer wrote:
>>
>>> I think this broken --enable-static-pie builds for some reason:
>>>
>>> In file included from bench-timing-type.c:19:0:
>>> bench-timing.h:19:0: error: "attribute_hidden" redefined [-Werror]
>>> #define attribute_hidden
>>> In file included from <command-line>:0:0:
>>> ./../include/libc-symbols.h:361:0: note: this is the location of the
>>> previous definition
>>> # define attribute_hidden __attribute__ ((visibility ("hidden")))
>>>
>>> Any suggestions how to fix this? Just stick an #undef attribute_hidden
>>> in front of it?
There's a bench-timing.h change in f1c8185d345 that wasn't part of the
posted patch, that's what is causing the breakage.
>> Hmm that looks like a preincluded file, which is done just before we
>> define _ISOMAC... So I wonder whether it would be better to fix
>> Makeconfig to pre-includes after the CPPFLAGS rather than in the
>> middle of it?
>
> I think Zack's suggestion regarding the change of module will help here.
I suppose it would (once someone tries to figure out the nonlib
behaviour again) but the easier fix for now should be to just undef it
before defining.
Siddhesh
@@ -125,7 +125,7 @@ ifndef BENCH_DURATION
BENCH_DURATION := 10
endif
-CPPFLAGS-nonlib += -DDURATION=$(BENCH_DURATION)
+CPPFLAGS-nonlib += -DDURATION=$(BENCH_DURATION) -D_ISOMAC
# Use clock_gettime to measure performance of functions. The default is to use
# HP_TIMING if it is available.
@@ -22,6 +22,7 @@
#include <stdlib.h>
#include <locale.h>
#include <unistd.h>
+#include <sys/stat.h>
#include "json-lib.h"
#include "bench-timing.h"
#include <string.h>
@@ -19,6 +19,16 @@
#include <getopt.h>
#include <sys/cdefs.h>
+/* We are compiled under _ISOMAC, so libc-symbols.h does not do this
+ for us. */
+#include "config.h"
+#ifdef HAVE_CC_INHIBIT_LOOP_TO_LIBCALL
+# define inhibit_loop_to_libcall \
+ __attribute__ ((__optimize__ ("-fno-tree-loop-distribute-patterns")))
+#else
+# define inhibit_loop_to_libcall
+#endif
+
typedef struct
{
const char *name;
@@ -22,6 +22,7 @@
#define STRSTR simple_strstr
+#define libc_hidden_builtin_def(X)
#include "../string/strstr.c"
@@ -42,7 +42,7 @@ oldstrtok (char *s, const char *delim)
s = strpbrk (token, delim);
if (s == NULL)
/* This token finishes the string. */
- olds = __rawmemchr (token, '\0');
+ olds = rawmemchr (token, '\0');
else
{
/* Terminate the token and make OLDS point past it. */