[PATCH] Use correct includes in benchtests

[PATCH] Use correct includes in benchtests

[Wilco Dijkstra]

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.

--

diff --git a/benchtests/Makefile b/benchtests/Makefile
index 419853f0b9cb91cde73a76d97cc5b998e4420e8e..5da3fd9fe5ac3ef855e32cde36b852d225ba2bf1 100644
--- a/benchtests/Makefile
+++ b/benchtests/Makefile
@@ -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.
diff --git a/benchtests/bench-strcoll.c b/benchtests/bench-strcoll.c
index 4a0b871048e885b87b7e512d7a8fd89f4ab04f28..ac7f32fc6a4f2b2c2a4bf6008dfcad8ae59557bb 100644
--- a/benchtests/bench-strcoll.c
+++ b/benchtests/bench-strcoll.c
@@ -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>
diff --git a/benchtests/bench-string.h b/benchtests/bench-string.h
index 6be6956f4bdb343052e994dbf41105d6bc9c2e0a..94aaafdaf2e1319b8cebb5baaf51f02b52182d5e 100644
--- a/benchtests/bench-string.h
+++ b/benchtests/bench-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;
diff --git a/benchtests/bench-strstr.c b/benchtests/bench-strstr.c
index 86d5e829dae4b1f2e4969a571a2a75dff46d2477..c30cd1078586fe66c9177707ba603bb6ba0476a7 100644
--- a/benchtests/bench-strstr.c
+++ b/benchtests/bench-strstr.c
@@ -22,6 +22,7 @@
 
 
 #define STRSTR simple_strstr
+#define libc_hidden_builtin_def(X)
 #include "../string/strstr.c"
 
 
diff --git a/benchtests/bench-strtok.c b/benchtests/bench-strtok.c
index d01c57669cb286f82986b77a97781dd9b251a640..ba8c2dcc5630e9c67908d3feb66e30c76b51d36d 100644
--- a/benchtests/bench-strtok.c
+++ b/benchtests/bench-strtok.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.  */

Re: [PATCH] Use correct includes in benchtests

[Siddhesh Poyarekar]

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

Re: [PATCH] Use correct includes in benchtests

[Zack Weinberg]

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

Re: [PATCH] Use correct includes in benchtests

[Siddhesh Poyarekar]

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

Re: [PATCH] Use correct includes in benchtests

[Florian Weimer]

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

Re: [PATCH] Use correct includes in benchtests

[Wilco Dijkstra]

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

Re: [PATCH] Use correct includes in benchtests

[Florian Weimer]

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

Re: [PATCH] Use correct includes in benchtests

[Siddhesh Poyarekar]

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

Re: [PATCH] Use correct includes in benchtests

[Wilco Dijkstra]

Siddhesh Poyarekar wrote:

> There's a bench-timing.h change in f1c8185d345 that wasn't part of the
> posted patch, that's what is causing the breakage.

That's necessary to fix the use of the attribute by hp-timing.h in some
configurations. I've added the undefine which should fix it for all cases:

Add an undefine of attribute_hidden since it may be defined in some cases
(it must be defined since it is used by some hp-timing configurations).

ChangeLog:
2018-03-19  Wilco Dijkstra  <wdijkstr@arm.com>

	* benchtests/bench-timing.h (attribute_hidden): Undefine.
--
diff --git a/benchtests/bench-timing.h b/benchtests/bench-timing.h
index b9ea04bb02c531f2025ee502c2874bc34dddade6..96cde1e8be2e0c2f40eb80159375dc89ca6a3376 100644
--- a/benchtests/bench-timing.h
+++ b/benchtests/bench-timing.h
@@ -16,6 +16,7 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
+#undef attribute_hidden
 #define attribute_hidden
 #include <hp-timing.h>
 #include <stdint.h>

    

Re: [PATCH] Use correct includes in benchtests

[Florian Weimer]

On 03/19/2018 12:03 PM, Wilco Dijkstra wrote:
> 	* benchtests/bench-timing.h (attribute_hidden): Undefine.

Okay, please commit.

Thanks,
Florian