diff mbox

[v2] intl: reintroduce unintentionally disabled optimization

Message ID 20160124181337.GA8527@altlinux.org
State Accepted, archived
Delegated to: Dmitry Levin
Headers

Commit Message

Dmitry V. Levin Jan. 24, 2016, 6:13 p.m. UTC 
  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(+)

Comments

Mike Frysinger Feb. 22, 2016, 9:17 a.m. UTC | #1 
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
Dmitry V. Levin Feb. 22, 2016, 12:01 p.m. UTC | #2 
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
Andreas Schwab Feb. 22, 2016, 12:51 p.m. UTC | #3 
"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.
Dmitry V. Levin Feb. 22, 2016, 12:59 p.m. UTC | #4 
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?
Andreas Schwab Feb. 22, 2016, 1:09 p.m. UTC | #5 
"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.
Dmitry V. Levin Feb. 22, 2016, 2:07 p.m. UTC | #6 
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?
Andreas Schwab Feb. 22, 2016, 2:32 p.m. UTC | #7 
"Dmitry V. Levin" <ldv@altlinux.org> writes:

> Is PATCH v2 OK then?

Yes, it is ok.

Andreas.
diff mbox

Patch

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