Finish __builtin_expect -> __glibc_{un}likely cleanup in elf/dl-load.c
diff mbox

Message ID ye6qeh1o7ab8.fsf@elbrus2.mtv.corp.google.com
State New
Headers show

Commit Message

Paul Pluzhnikov March 27, 2014, 12:30 a.m. UTC
Greetings,

This patch cleans up remaining uses of __builtin_expect.

It does change elf/dl-load.o in non-trivial ways, but
- the tests still pass on Linux/x86_64 and
- is much smaller, and easier to verify.

Thanks,
--

2014-03-26  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* elf/dl-load.c: Finish __builtin_expect into __glibc_{un}likely
        conversion.

Comments

Ondřej Bílka March 27, 2014, 11:42 a.m. UTC | #1
On Wed, Mar 26, 2014 at 05:30:03PM -0700, Paul Pluzhnikov wrote:
> Greetings,
> 
> This patch cleans up remaining uses of __builtin_expect.
> 
> It does change elf/dl-load.o in non-trivial ways, but
> - the tests still pass on Linux/x86_64 and
> - is much smaller, and easier to verify.
>
Looks mostly ok.

> @@ -2104,8 +2103,7 @@ _dl_map_object (struct link_map *loader, const char *name,
>        /* If the requested name matches the soname of a loaded object,
>  	 use that object.  Elide this check for names that have not
>  	 yet been opened.  */
> -      if (__glibc_unlikely (l->l_faked != 0)
> -	  || __builtin_expect (l->l_removed, 0) != 0)
> +      if (__glibc_unlikely (l->l_faked != 0 || l->l_removed != 0))
>  	continue;
>        if (!_dl_name_match_p (name, l))
>  	{

Here it might decrease performance (see below), do you have profile data it migth
make sense even to drop this one.

As written now its optimized to

if (__glibc_unlikely (l->l_faked != 0)
  continue;
if (__builtin_expect (l->l_removed, 0) != 0)
  continue;

with your change compiler could assume that first part happens in 49%
time and 49% for other part. So its better to trasform that expression
that requires only one jump, here best one is

if (__glibc_unlikely (l->l_faked | l->l_removed != 0))
  continue;


> @@ -2230,7 +2228,7 @@ _dl_map_object (struct link_map *loader, const char *name,
>  
>  #ifdef USE_LDCONFIG
>        if (fd == -1
> -	  && (__builtin_expect (! (mode & __RTLD_SECURE), 1)
> +	  && (__glibc_likely ((mode & __RTLD_SECURE) == 0)
>  	      || ! INTUSE(__libc_enable_secure))
>  	  && __glibc_likely (GLRO(dl_inhibit_cache) == 0))
>  	{

Extra parens here.
Andreas Schwab March 27, 2014, 11:51 a.m. UTC | #2
Ondřej Bílka <neleai@seznam.cz> writes:

> if (__glibc_unlikely (l->l_faked != 0)
>   continue;
> if (__builtin_expect (l->l_removed, 0) != 0)
>   continue;

__builtin_expect should really only be used in boolean context, ie. this
should be written as:

    if (__builtin_expect (l->l_removed != 0, 0))
      continue;

Andreas.
Paul Pluzhnikov March 27, 2014, 4:42 p.m. UTC | #3
On Thu, Mar 27, 2014 at 4:42 AM, Ondřej Bílka <neleai@seznam.cz> wrote:

> On Wed, Mar 26, 2014 at 05:30:03PM -0700, Paul Pluzhnikov wrote:

>> @@ -2104,8 +2103,7 @@ _dl_map_object (struct link_map *loader, const char *name,
>>        /* If the requested name matches the soname of a loaded object,
>>        use that object.  Elide this check for names that have not
>>        yet been opened.  */
>> -      if (__glibc_unlikely (l->l_faked != 0)
>> -       || __builtin_expect (l->l_removed, 0) != 0)
>> +      if (__glibc_unlikely (l->l_faked != 0 || l->l_removed != 0))
>>       continue;
>>        if (!_dl_name_match_p (name, l))
>>       {
...
> if (__glibc_unlikely (l->l_faked | l->l_removed != 0))
>   continue;

Done.

>> @@ -2230,7 +2228,7 @@ _dl_map_object (struct link_map *loader, const char *name,
>>
>>  #ifdef USE_LDCONFIG
>>        if (fd == -1
>> -       && (__builtin_expect (! (mode & __RTLD_SECURE), 1)
>> +       && (__glibc_likely ((mode & __RTLD_SECURE) == 0)
>>             || ! INTUSE(__libc_enable_secure))
>>         && __glibc_likely (GLRO(dl_inhibit_cache) == 0))
>>       {
>
> Extra parens here.

Are there? I don't see them :-(

Thanks,
Ondřej Bílka March 27, 2014, 5:31 p.m. UTC | #4
On Thu, Mar 27, 2014 at 09:42:54AM -0700, Paul Pluzhnikov wrote:
> 
> >> @@ -2230,7 +2228,7 @@ _dl_map_object (struct link_map *loader, const char *name,
> >>
> >>  #ifdef USE_LDCONFIG
> >>        if (fd == -1
> >> -       && (__builtin_expect (! (mode & __RTLD_SECURE), 1)
> >> +       && (__glibc_likely ((mode & __RTLD_SECURE) == 0)
> >>             || ! INTUSE(__libc_enable_secure))
> >>         && __glibc_likely (GLRO(dl_inhibit_cache) == 0))
> >>       {
> >
> > Extra parens here.
> 
> Are there? I don't see them :-(
> 
Never mind, when I saw that I forgotten that & has weird precedence.

Patch
diff mbox

diff --git a/elf/dl-load.c b/elf/dl-load.c
index 2d15e41..0ae0ad1 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1269,7 +1269,7 @@  cannot allocate TLS data structures for initial thread");
     /* Length of the sections to be loaded.  */
     maplength = loadcmds[nloadcmds - 1].allocend - c->mapstart;
 
-    if (__builtin_expect (type, ET_DYN) == ET_DYN)
+    if (__glibc_likely (type == ET_DYN))
       {
 	/* This is a position-independent shared object.  We can let the
 	   kernel map it anywhere it likes, but we must have space for all
@@ -1767,14 +1767,13 @@  open_verify (const char *name, struct filebuf *fbp, struct link_map *loader,
 	}
 
       /* See whether the ELF header is what we expect.  */
-      if (__builtin_expect (! VALID_ELF_HEADER (ehdr->e_ident, expected,
+      if (__glibc_unlikely (! VALID_ELF_HEADER (ehdr->e_ident, expected,
 						EI_ABIVERSION)
 			    || !VALID_ELF_ABIVERSION (ehdr->e_ident[EI_OSABI],
 						      ehdr->e_ident[EI_ABIVERSION])
 			    || memcmp (&ehdr->e_ident[EI_PAD],
 				       &expected[EI_PAD],
-				       EI_NIDENT - EI_PAD) != 0,
-			    0))
+				       EI_NIDENT - EI_PAD) != 0))
 	{
 	  /* Something is wrong.  */
 	  const Elf32_Word *magp = (const void *) ehdr->e_ident;
@@ -1832,10 +1831,10 @@  open_verify (const char *name, struct filebuf *fbp, struct link_map *loader,
 	  errstring = N_("ELF file version does not match current one");
 	  goto call_lose;
 	}
-      if (! __builtin_expect (elf_machine_matches_host (ehdr), 1))
+      if (! __glibc_likely (elf_machine_matches_host (ehdr)))
 	goto close_and_out;
-      else if (__builtin_expect (ehdr->e_type, ET_DYN) != ET_DYN
-	       && __builtin_expect (ehdr->e_type, ET_EXEC) != ET_EXEC)
+      else if (__glibc_unlikely (ehdr->e_type != ET_DYN
+				 && ehdr->e_type != ET_EXEC))
 	{
 	  errstring = N_("only ET_DYN and ET_EXEC can be loaded");
 	  goto call_lose;
@@ -2104,8 +2103,7 @@  _dl_map_object (struct link_map *loader, const char *name,
       /* If the requested name matches the soname of a loaded object,
 	 use that object.  Elide this check for names that have not
 	 yet been opened.  */
-      if (__glibc_unlikely (l->l_faked != 0)
-	  || __builtin_expect (l->l_removed, 0) != 0)
+      if (__glibc_unlikely (l->l_faked != 0 || l->l_removed != 0))
 	continue;
       if (!_dl_name_match_p (name, l))
 	{
@@ -2230,7 +2228,7 @@  _dl_map_object (struct link_map *loader, const char *name,
 
 #ifdef USE_LDCONFIG
       if (fd == -1
-	  && (__builtin_expect (! (mode & __RTLD_SECURE), 1)
+	  && (__glibc_likely ((mode & __RTLD_SECURE) == 0)
 	      || ! INTUSE(__libc_enable_secure))
 	  && __glibc_likely (GLRO(dl_inhibit_cache) == 0))
 	{