diff mbox

intl: reintroduce unintentionally disabled optimization

Message ID 20160124000754.GA27321@altlinux.org
State New
Headers show

Commit Message

Dmitry V. Levin Jan. 24, 2016, 12:07 a.m. UTC 
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(-)

Comments

Andreas Schwab Jan. 24, 2016, 8:46 a.m. UTC | #1 
"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.
Florian Weimer Feb. 22, 2016, 12:31 p.m. UTC | #2 
* 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?
Dmitry V. Levin Feb. 22, 2016, 1:06 p.m. UTC | #3 
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
Florian Weimer Feb. 22, 2016, 2:05 p.m. UTC | #4 
* 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?
Dmitry V. Levin Feb. 22, 2016, 2:23 p.m. UTC | #5 
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?
Andreas Schwab Feb. 22, 2016, 2:31 p.m. UTC | #6 
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.
Mike Frysinger Feb. 22, 2016, 4:26 p.m. UTC | #7 
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
Joseph Myers Feb. 22, 2016, 6:25 p.m. UTC | #8 
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).
diff mbox

Patch

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