[PATCH] intl: reintroduce unintentionally disabled optimization

[PATCH] intl: reintroduce unintentionally disabled optimization

[Dmitry V. Levin]

HAVE_BUILTIN_EXPECT macro was removed by commit glibc-2.14-280-g3ce1f29,
but then unintentionally reintroduced during merge with GNU gettext
0.19.3 by commit glibc-2.20-324-g6d24885, effectively disabling all
optimization based on __builtin_expect.  As intl files are also part
of GNU gettext, HAVE_BUILTIN_EXPECT macro cannot be removed, so guard
its use with _LIBC macro.

[BZ #19512]
* intl/gettextP.h (__builtin_expect): Define only if
[!_LIBC && !HAVE_BUILTIN_EXPECT].
* intl/loadinfo.h (__builtin_expect): Likewise.
---
 intl/gettextP.h | 2 +-
 intl/loadinfo.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/intl/gettextP.h b/intl/gettextP.h
index 8c74bc5..83d9395 100644
--- a/intl/gettextP.h
+++ b/intl/gettextP.h
@@ -99,7 +99,7 @@ extern char *libintl_dcigettext (const char *__domainname,
 
 /* Tell the compiler when a conditional or integer expression is
    almost always true or almost always false.  */
-#ifndef HAVE_BUILTIN_EXPECT
+#if !defined _LIBC && !defined HAVE_BUILTIN_EXPECT
 # define __builtin_expect(expr, val) (expr)
 #endif
 
diff --git a/intl/loadinfo.h b/intl/loadinfo.h
index 2e15f93..025fc3a 100644
--- a/intl/loadinfo.h
+++ b/intl/loadinfo.h
@@ -40,7 +40,7 @@
 
 /* Tell the compiler when a conditional or integer expression is
    almost always true or almost always false.  */
-#ifndef HAVE_BUILTIN_EXPECT
+#if !defined _LIBC && !defined HAVE_BUILTIN_EXPECT
 # define __builtin_expect(expr, val) (expr)
 #endif
 
-- 
ldv

Re: [PATCH] intl: reintroduce unintentionally disabled optimization

[Andreas Schwab]

"Dmitry V. Levin" <ldv@altlinux.org> writes:

> HAVE_BUILTIN_EXPECT macro was removed by commit glibc-2.14-280-g3ce1f29,
> but then unintentionally reintroduced during merge with GNU gettext
> 0.19.3 by commit glibc-2.20-324-g6d24885, effectively disabling all
> optimization based on __builtin_expect.  As intl files are also part
> of GNU gettext, HAVE_BUILTIN_EXPECT macro cannot be removed, so guard
> its use with _LIBC macro.

How about defining HAVE_BUILTIN_EXPECT in config.h.in?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

[PATCH v2] intl: reintroduce unintentionally disabled optimization

[Dmitry V. Levin]

HAVE_BUILTIN_EXPECT macro was removed by commit glibc-2.14-280-g3ce1f29,
but then its use was unintentionally reintroduced during merge with GNU
gettext 0.19.3 by commit glibc-2.20-324-g6d24885, effectively disabling
all optimization based on __builtin_expect.  As intl files are also part
of GNU gettext, HAVE_BUILTIN_EXPECT macro cannot be removed, so define
it unconditionally in config.h.in instead.

[BZ #19512]
* config.h.in (HAVE_BUILTIN_EXPECT): New macro.
---
 config.h.in | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/config.h.in b/config.h.in
index ec9c8bc..13c0044 100644
--- a/config.h.in
+++ b/config.h.in
@@ -37,6 +37,11 @@
 /* Define if static NSS modules are wanted.  */
 #undef	DO_STATIC_NSS
 
+/* Assume that the compiler supports __builtin_expect.
+   This macro is necessary for proper compilation of code
+   shared between GNU libc and GNU gettext projects.  */
+#define HAVE_BUILTIN_EXPECT 1
+
 /* Define if the compiler supports __builtin_memset.  */
 #undef	HAVE_BUILTIN_MEMSET
 
-- 
ldv

Re: [PATCH v2] intl: reintroduce unintentionally disabled optimization

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]

On 24 Jan 2016 21:13, Dmitry V. Levin wrote:
> HAVE_BUILTIN_EXPECT macro was removed by commit glibc-2.14-280-g3ce1f29,
> but then its use was unintentionally reintroduced during merge with GNU
> gettext 0.19.3 by commit glibc-2.20-324-g6d24885, effectively disabling
> all optimization based on __builtin_expect.  As intl files are also part
> of GNU gettext, HAVE_BUILTIN_EXPECT macro cannot be removed, so define
> it unconditionally in config.h.in instead.
> 
> [BZ #19512]
> * config.h.in (HAVE_BUILTIN_EXPECT): New macro.
> ---
>  config.h.in | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/config.h.in b/config.h.in
> index ec9c8bc..13c0044 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -37,6 +37,11 @@
>  /* Define if static NSS modules are wanted.  */
>  #undef	DO_STATIC_NSS
>  
> +/* Assume that the compiler supports __builtin_expect.
> +   This macro is necessary for proper compilation of code
> +   shared between GNU libc and GNU gettext projects.  */
> +#define HAVE_BUILTIN_EXPECT 1
> +
>  /* Define if the compiler supports __builtin_memset.  */
>  #undef	HAVE_BUILTIN_MEMSET

shouldn't this be an AC_DEFINE in configure.ac instead ?
or do we not have a policy for that ?

it doesn't matter *that* much as long as we avoid autoheader,
although it would be nice if we didn't ...
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] intl: reintroduce unintentionally disabled optimization

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 1689 bytes --]

On Mon, Feb 22, 2016 at 04:17:46AM -0500, Mike Frysinger wrote:
> On 24 Jan 2016 21:13, Dmitry V. Levin wrote:
> > HAVE_BUILTIN_EXPECT macro was removed by commit glibc-2.14-280-g3ce1f29,
> > but then its use was unintentionally reintroduced during merge with GNU
> > gettext 0.19.3 by commit glibc-2.20-324-g6d24885, effectively disabling
> > all optimization based on __builtin_expect.  As intl files are also part
> > of GNU gettext, HAVE_BUILTIN_EXPECT macro cannot be removed, so define
> > it unconditionally in config.h.in instead.
> > 
> > [BZ #19512]
> > * config.h.in (HAVE_BUILTIN_EXPECT): New macro.
> > ---
> >  config.h.in | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/config.h.in b/config.h.in
> > index ec9c8bc..13c0044 100644
> > --- a/config.h.in
> > +++ b/config.h.in
> > @@ -37,6 +37,11 @@
> >  /* Define if static NSS modules are wanted.  */
> >  #undef	DO_STATIC_NSS
> >  
> > +/* Assume that the compiler supports __builtin_expect.
> > +   This macro is necessary for proper compilation of code
> > +   shared between GNU libc and GNU gettext projects.  */
> > +#define HAVE_BUILTIN_EXPECT 1
> > +
> >  /* Define if the compiler supports __builtin_memset.  */
> >  #undef	HAVE_BUILTIN_MEMSET
> 
> shouldn't this be an AC_DEFINE in configure.ac instead ?
> or do we not have a policy for that ?

We certainly do have some macros defined directly in this file:
$ grep '#define' config.h.in
#define HAVE_IFUNC 0
#define	HAVE_STRERROR	1
#define HAVE_REGEX 1
#define ARM_PCREL_MOVW_OK 0
#define HAVE_PT_CHOWN 0
#define HAVE_BUILTIN_TRAP 0
#define HAVE_PPC_FCFID 0
#define HAVE_PPC_FCTIDZ 0


-- 
ldv

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] intl: reintroduce unintentionally disabled optimization

[Florian Weimer]

* Andreas Schwab:

> "Dmitry V. Levin" <ldv@altlinux.org> writes:
>
>> HAVE_BUILTIN_EXPECT macro was removed by commit glibc-2.14-280-g3ce1f29,
>> but then unintentionally reintroduced during merge with GNU gettext
>> 0.19.3 by commit glibc-2.20-324-g6d24885, effectively disabling all
>> optimization based on __builtin_expect.  As intl files are also part
>> of GNU gettext, HAVE_BUILTIN_EXPECT macro cannot be removed, so guard
>> its use with _LIBC macro.
>
> How about defining HAVE_BUILTIN_EXPECT in config.h.in?

Is config.h.in regenerated by autoreconf and similar tools?

Re: [PATCH v2] intl: reintroduce unintentionally disabled optimization

[Andreas Schwab]

"Dmitry V. Levin" <ldv@altlinux.org> writes:

> We certainly do have some macros defined directly in this file:
> $ grep '#define' config.h.in
> #define HAVE_IFUNC 0
> #define	HAVE_STRERROR	1
> #define HAVE_REGEX 1
> #define ARM_PCREL_MOVW_OK 0
> #define HAVE_PT_CHOWN 0
> #define HAVE_BUILTIN_TRAP 0
> #define HAVE_PPC_FCFID 0
> #define HAVE_PPC_FCTIDZ 0

Most of these (those defined to 0) are the templates used by AC_DEFINE.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v2] intl: reintroduce unintentionally disabled optimization

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

On Mon, Feb 22, 2016 at 01:51:02PM +0100, Andreas Schwab wrote:
> 
> > We certainly do have some macros defined directly in this file:
> > $ grep '#define' config.h.in
> > #define HAVE_IFUNC 0
> > #define	HAVE_STRERROR	1
> > #define HAVE_REGEX 1
> > #define ARM_PCREL_MOVW_OK 0
> > #define HAVE_PT_CHOWN 0
> > #define HAVE_BUILTIN_TRAP 0
> > #define HAVE_PPC_FCFID 0
> > #define HAVE_PPC_FCTIDZ 0
> 
> Most of these (those defined to 0) are the templates used by AC_DEFINE.

OK, so what's the preferred way of adding new macros
like HAVE_BUILTIN_EXPECT and HAVE_REGEX?  


-- 
ldv

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] intl: reintroduce unintentionally disabled optimization

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

On Mon, Feb 22, 2016 at 01:31:50PM +0100, Florian Weimer wrote:
> * Andreas Schwab:
> > Dmitry V. Levin writes:
> >
> >> HAVE_BUILTIN_EXPECT macro was removed by commit glibc-2.14-280-g3ce1f29,
> >> but then unintentionally reintroduced during merge with GNU gettext
> >> 0.19.3 by commit glibc-2.20-324-g6d24885, effectively disabling all
> >> optimization based on __builtin_expect.  As intl files are also part
> >> of GNU gettext, HAVE_BUILTIN_EXPECT macro cannot be removed, so guard
> >> its use with _LIBC macro.
> >
> > How about defining HAVE_BUILTIN_EXPECT in config.h.in?
> 
> Is config.h.in regenerated by autoreconf and similar tools?

No, it's supported manually and therefore permanently out of sync.
For example,

$ grep -i strerror config.h.in 
#undef	HAVE_STRERROR
#define	HAVE_STRERROR	1
find -name configure.ac | xargs grep -i strerror | wc -l
0


-- 
ldv

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] intl: reintroduce unintentionally disabled optimization

[Andreas Schwab]

"Dmitry V. Levin" <ldv@altlinux.org> writes:

> On Mon, Feb 22, 2016 at 01:51:02PM +0100, Andreas Schwab wrote:
>> 
>> > We certainly do have some macros defined directly in this file:
>> > $ grep '#define' config.h.in
>> > #define HAVE_IFUNC 0
>> > #define	HAVE_STRERROR	1
>> > #define HAVE_REGEX 1
>> > #define ARM_PCREL_MOVW_OK 0
>> > #define HAVE_PT_CHOWN 0
>> > #define HAVE_BUILTIN_TRAP 0
>> > #define HAVE_PPC_FCFID 0
>> > #define HAVE_PPC_FCTIDZ 0
>> 
>> Most of these (those defined to 0) are the templates used by AC_DEFINE.
>
> OK, so what's the preferred way of adding new macros
> like HAVE_BUILTIN_EXPECT and HAVE_REGEX?  

For case like HAVE_BUILTIN_EXPECT there is not much point in using
AC_DEFINE.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] intl: reintroduce unintentionally disabled optimization

[Florian Weimer]

* Dmitry V. Levin:

>> Is config.h.in regenerated by autoreconf and similar tools?
>
> No, it's supported manually and therefore permanently out of sync.
> For example,
>
> $ grep -i strerror config.h.in 
> #undef	HAVE_STRERROR
> #define	HAVE_STRERROR	1
> find -name configure.ac | xargs grep -i strerror | wc -l
> 0

Can we have autoreconf automatically generate config.h.in, like other
GNU projects do?

Re: [PATCH v2] intl: reintroduce unintentionally disabled optimization

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 860 bytes --]

On Mon, Feb 22, 2016 at 02:09:48PM +0100, Andreas Schwab wrote:
> Dmitry V. Levin writes:
> > On Mon, Feb 22, 2016 at 01:51:02PM +0100, Andreas Schwab wrote:
> >> 
> >> > We certainly do have some macros defined directly in this file:
> >> > $ grep '#define' config.h.in
> >> > #define HAVE_IFUNC 0
> >> > #define	HAVE_STRERROR	1
> >> > #define HAVE_REGEX 1
> >> > #define ARM_PCREL_MOVW_OK 0
> >> > #define HAVE_PT_CHOWN 0
> >> > #define HAVE_BUILTIN_TRAP 0
> >> > #define HAVE_PPC_FCFID 0
> >> > #define HAVE_PPC_FCTIDZ 0
> >> 
> >> Most of these (those defined to 0) are the templates used by AC_DEFINE.
> >
> > OK, so what's the preferred way of adding new macros
> > like HAVE_BUILTIN_EXPECT and HAVE_REGEX?  
> 
> For case like HAVE_BUILTIN_EXPECT there is not much point in using
> AC_DEFINE.

Is PATCH v2 OK then?


-- 
ldv

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Q: generate config.h.in automatically (was: Re: [PATCH] intl: reintroduce unintentionally disabled optimization)

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

On Mon, Feb 22, 2016 at 03:05:47PM +0100, Florian Weimer wrote:
> * Dmitry V. Levin:
> >> Is config.h.in regenerated by autoreconf and similar tools?
> >
> > No, it's supported manually and therefore permanently out of sync.
> > For example,
> >
> > $ grep -i strerror config.h.in 
> > #undef	HAVE_STRERROR
> > #define	HAVE_STRERROR	1
> > find -name configure.ac | xargs grep -i strerror | wc -l
> > 0
> 
> Can we have autoreconf automatically generate config.h.in, like other
> GNU projects do?

I suppose we can use autoheader like other projects do.

I can't recollect why don't we use it in the first place.  Is there
anybody around who can sched some light on the history of not using
autoheader in glibc?


-- 
ldv

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] intl: reintroduce unintentionally disabled optimization

[Andreas Schwab]

Florian Weimer <fw@deneb.enyo.de> writes:

> Can we have autoreconf automatically generate config.h.in, like other
> GNU projects do?

That doesn't work with our use of sysdep configure scripts.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v2] intl: reintroduce unintentionally disabled optimization

[Andreas Schwab]

"Dmitry V. Levin" <ldv@altlinux.org> writes:

> Is PATCH v2 OK then?

Yes, it is ok.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] intl: reintroduce unintentionally disabled optimization

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]

On 22 Feb 2016 15:31, Andreas Schwab wrote:
> Florian Weimer <fw@deneb.enyo.de> writes:
> > Can we have autoreconf automatically generate config.h.in, like other
> > GNU projects do?
> 
> That doesn't work with our use of sysdep configure scripts.

autoheader isn't all-or-nothing.  for the small handful of defines that
live in subdirs, we can call AH_BOTTOM in the toplevel configure.ac file.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] intl: reintroduce unintentionally disabled optimization

[Joseph Myers]

On Mon, 22 Feb 2016, Florian Weimer wrote:

> * Dmitry V. Levin:
> 
> >> Is config.h.in regenerated by autoreconf and similar tools?
> >
> > No, it's supported manually and therefore permanently out of sync.
> > For example,
> >
> > $ grep -i strerror config.h.in 
> > #undef	HAVE_STRERROR
> > #define	HAVE_STRERROR	1
> > find -name configure.ac | xargs grep -i strerror | wc -l
> > 0
> 
> Can we have autoreconf automatically generate config.h.in, like other
> GNU projects do?

The complications with that are (a) manually written code in there such as 
the #error calls and macros present for the benefit of code shared with 
other projects (e.g. asserting that glibc has certain functionality, when 
it's not possible to test for it at configure time when building glibc), 
(b) macros present only for sysdeps configure scripts and (c) conditionals 
around some of the macros.

Now, we might formulate a strategy for eliminating such problem cases, 
which could be done incrementally.  For example:

* Some of the macros aren't actually used anywhere and could just be 
removed.

* Manually written content that doesn't actually depend on configure tests 
might better go in include/libc-symbols.h, which is what's implicitly 
preincluded via CPPFLAGS (except for compilations for the build system, 
which include config.h directly - only really OK if there are no conflicts 
between testing for given features on the two systems).

* Are the _LIBC conditionals actually needed?

* Bug 14068 discusses the issue of separation between machine-dependent 
and machine-independent pieces, though I don't know how existing auto* 
features might best be used to generate and use multiple config.h.in 
fragments.  Anything that is an architecture / OS property not depending 
on compiler / binutils features tested at configure time can go in sysdeps 
headers instead of config.h.in (except for the cases that are for use in 
conditionals in shlib-versions files, to avoid needing extra sysdeps 
directories in those cases).

* <https://sourceware.org/ml/libc-alpha/2014-06/msg00784.html> deals with 
the USE_REGPARMS / internal_function issue.

* Any HAVE_* needed only in certain directories for shared code could be 
defined in those directories (cf. timezone/Makefile defining various 
macros to configure the code from tzcode).

-- 
Joseph S. Myers
joseph@codesourcery.com