diff mbox series

i386: Use __glibc_likely/__glibc_likely in dl-machine.h

Message ID 20180205122807.GA12157@gmail.com
State New
Headers show
Series i386: Use __glibc_likely/__glibc_likely in dl-machine.h | expand

Commit Message

H.J. Lu Feb. 5, 2018, 12:28 p.m. UTC 
The differences in elf/dl-reloc.os are

Comments

Dmitry V. Levin Feb. 5, 2018, 8:15 p.m. UTC | #1 
On Mon, Feb 05, 2018 at 04:28:07AM -0800, H.J. Lu wrote:
> The differences in elf/dl-reloc.os are
> 
> --- before    	2018-02-05 03:53:31.970492246 -0800
> +++ after     	2018-02-05 03:53:49.719902340 -0800
> @@ -1202,9 +1202,9 @@ _dl_relocate_object:
>  	movl	-60(%ebp), %eax
>  	testl	%eax, %eax
>  	je	.L249
> -	movl	8(%eax), %eax
> -	movl	8(%ebx), %esi
> -	cmpl	%esi, %eax
> +	movl	8(%eax), %esi
> +	movl	8(%ebx), %eax
> +	cmpl	%eax, %esi
>  	ja	.L284
>  	jb	.L707
>  .L285:
> @@ -2255,7 +2255,7 @@ _dl_relocate_object:
>  	cmpl	$6, %edi
>  	movl	$4, %edx
>  	je	.L132
> -	cmpl	%ecx, %eax
> +	cmpl	%eax, %ecx
>  	je	.L350
>  	cmpl	$7, %edi
>  	je	.L419
> @@ -2735,7 +2735,7 @@ _dl_relocate_object:
>  	je	.L120
>  .L121:
>  	movl	-96(%ebp), %edx
> -	movl	$640, 8(%esp)
> +	movl	$639, 8(%esp)
>  	leal	__PRETTY_FUNCTION__.9431@GOTOFF(%edx), %eax
>  	movl	%eax, 12(%esp)
>  	leal	.LC9@GOTOFF(%edx), %eax
> @@ -3454,10 +3454,10 @@ _dl_relocate_object:
>  	movl	-152(%ebp), %eax
>  	movl	%eax, 4(%esp)
>  	call	_dl_dprintf
> -	movl	-60(%ebp), %eax
> -	movl	8(%ebx), %esi
> +	movl	8(%ebx), %eax
> +	movl	-60(%ebp), %ebx
>  	movl	-112(%ebp), %edx
> -	movl	8(%eax), %eax
> +	movl	8(%ebx), %esi
>  	jmp	.L285
>  .L713:
>  	movl	%esi, (%esp)
> 
> I will check it in shortly.
> 
> H.J.
> ---
> 	* sysdeps/i386/dl-machine.h (elf_machine_rel): Replace
> 	__builtin_expect with __glibc_likely and __glibc_likely.
> 	(elf_machine_rela): Likewise.
> 	(elf_machine_lazy_rel): Likewise.

Why this patch has been committed without review?

I haven't looked into the change itself, but the commit message and
the changelog entry are very confusing because of
"__glibc_likely/__glibc_likely" and "__glibc_likely and __glibc_likely".
Zack Weinberg Feb. 5, 2018, 8:23 p.m. UTC | #2 
On Mon, Feb 5, 2018 at 7:28 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> The differences in elf/dl-reloc.os are
>
> --- before      2018-02-05 03:53:31.970492246 -0800
> +++ after       2018-02-05 03:53:49.719902340 -0800
> @@ -1202,9 +1202,9 @@ _dl_relocate_object:
>         movl    -60(%ebp), %eax
>         testl   %eax, %eax
>         je      .L249
> -       movl    8(%eax), %eax
> -       movl    8(%ebx), %esi
> -       cmpl    %esi, %eax
> +       movl    8(%eax), %esi
> +       movl    8(%ebx), %eax
> +       cmpl    %eax, %esi

Since __glibc_(un)likely just expand back to __builtin_expect, why are
there any assembly differences at all?
H.J. Lu Feb. 5, 2018, 8:52 p.m. UTC | #3 
On Mon, Feb 5, 2018 at 12:23 PM, Zack Weinberg <zackw@panix.com> wrote:
> On Mon, Feb 5, 2018 at 7:28 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> The differences in elf/dl-reloc.os are
>>
>> --- before      2018-02-05 03:53:31.970492246 -0800
>> +++ after       2018-02-05 03:53:49.719902340 -0800
>> @@ -1202,9 +1202,9 @@ _dl_relocate_object:
>>         movl    -60(%ebp), %eax
>>         testl   %eax, %eax
>>         je      .L249
>> -       movl    8(%eax), %eax
>> -       movl    8(%ebx), %esi
>> -       cmpl    %esi, %eax
>> +       movl    8(%eax), %esi
>> +       movl    8(%ebx), %eax
>> +       cmpl    %eax, %esi
>
> Since __glibc_(un)likely just expand back to __builtin_expect, why are
> there any assembly differences at all?

These changes are trigger by

@ -663,7 +662,8 @@ elf_machine_lazy_rel (struct link_map *map,
   /* Check for unexpected PLT reloc type.  */
   if (__glibc_likely (r_type == R_386_JMP_SLOT))
     {
-      if (__builtin_expect (map->l_mach.plt, 0) == 0)
+      /* Prelink has been deprecated.  */
+      if (__glibc_likely (map->l_mach.plt == 0))
   *reloc_addr += l_addr;
       else
   *reloc_addr = (map->l_mach.plt

Although they are equivalent, GCC treats

if (__builtin_expect (map->l_mach.plt, 0) == 0)

slightly differently from

if (__glibc_likely (map->l_mach.plt == 0))

such that the outputs aren't identical.
Zack Weinberg Feb. 5, 2018, 9:43 p.m. UTC | #4 
On Mon, Feb 5, 2018 at 3:52 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> Although they are equivalent, GCC treats
>
> if (__builtin_expect (map->l_mach.plt, 0) == 0)
>
> slightly differently from
>
> if (__glibc_likely (map->l_mach.plt == 0))
>
> such that the outputs aren't identical.

Thanks for the clarification.

zw
diff mbox series

Patch

--- before    	2018-02-05 03:53:31.970492246 -0800
+++ after     	2018-02-05 03:53:49.719902340 -0800
@@ -1202,9 +1202,9 @@  _dl_relocate_object:
 	movl	-60(%ebp), %eax
 	testl	%eax, %eax
 	je	.L249
-	movl	8(%eax), %eax
-	movl	8(%ebx), %esi
-	cmpl	%esi, %eax
+	movl	8(%eax), %esi
+	movl	8(%ebx), %eax
+	cmpl	%eax, %esi
 	ja	.L284
 	jb	.L707
 .L285:
@@ -2255,7 +2255,7 @@  _dl_relocate_object:
 	cmpl	$6, %edi
 	movl	$4, %edx
 	je	.L132
-	cmpl	%ecx, %eax
+	cmpl	%eax, %ecx
 	je	.L350
 	cmpl	$7, %edi
 	je	.L419
@@ -2735,7 +2735,7 @@  _dl_relocate_object:
 	je	.L120
 .L121:
 	movl	-96(%ebp), %edx
-	movl	$640, 8(%esp)
+	movl	$639, 8(%esp)
 	leal	__PRETTY_FUNCTION__.9431@GOTOFF(%edx), %eax
 	movl	%eax, 12(%esp)
 	leal	.LC9@GOTOFF(%edx), %eax
@@ -3454,10 +3454,10 @@  _dl_relocate_object:
 	movl	-152(%ebp), %eax
 	movl	%eax, 4(%esp)
 	call	_dl_dprintf
-	movl	-60(%ebp), %eax
-	movl	8(%ebx), %esi
+	movl	8(%ebx), %eax
+	movl	-60(%ebp), %ebx
 	movl	-112(%ebp), %edx
-	movl	8(%eax), %eax
+	movl	8(%ebx), %esi
 	jmp	.L285
 .L713:
 	movl	%esi, (%esp)

I will check it in shortly.

H.J.
---
	* sysdeps/i386/dl-machine.h (elf_machine_rel): Replace
	__builtin_expect with __glibc_likely and __glibc_likely.
	(elf_machine_rela): Likewise.
	(elf_machine_lazy_rel): Likewise.
---
 sysdeps/i386/dl-machine.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
index 5171499e33..eb8a6619e9 100644
--- a/sysdeps/i386/dl-machine.h
+++ b/sysdeps/i386/dl-machine.h
@@ -323,10 +323,9 @@  elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
       Elf32_Addr value = sym_map == NULL ? 0 : sym_map->l_addr + sym->st_value;
 
       if (sym != NULL
-	  && __builtin_expect (ELFW(ST_TYPE) (sym->st_info) == STT_GNU_IFUNC,
-			       0)
-	  && __builtin_expect (sym->st_shndx != SHN_UNDEF, 1)
-	  && __builtin_expect (!skip_ifunc, 1))
+	  && __glibc_unlikely (ELFW(ST_TYPE) (sym->st_info) == STT_GNU_IFUNC)
+	  && __glibc_likely (sym->st_shndx != SHN_UNDEF)
+	  && __glibc_likely (!skip_ifunc))
 	{
 # ifndef RTLD_BOOTSTRAP
 	  if (sym_map != map
@@ -456,8 +455,8 @@  elf_machine_rel (struct link_map *map, const Elf32_Rel *reloc,
 	    /* This can happen in trace mode if an object could not be
 	       found.  */
 	    break;
-	  if (__builtin_expect (sym->st_size > refsym->st_size, 0)
-	      || (__builtin_expect (sym->st_size < refsym->st_size, 0)
+	  if (__glibc_unlikely (sym->st_size > refsym->st_size)
+	      || (__glibc_unlikely(sym->st_size < refsym->st_size)
 		  && GLRO(dl_verbose)))
 	    {
 	      const char *strtab;
@@ -504,9 +503,9 @@  elf_machine_rela (struct link_map *map, const Elf32_Rela *reloc,
       Elf32_Addr value = sym == NULL ? 0 : sym_map->l_addr + sym->st_value;
 
       if (sym != NULL
-	  && __builtin_expect (sym->st_shndx != SHN_UNDEF, 1)
-	  && __builtin_expect (ELFW(ST_TYPE) (sym->st_info) == STT_GNU_IFUNC, 0)
-	  && __builtin_expect (!skip_ifunc, 1))
+	  && __glibc_likely (sym->st_shndx != SHN_UNDEF)
+	  && __glibc_unlikely (ELFW(ST_TYPE) (sym->st_info) == STT_GNU_IFUNC)
+	  && __glibc_likely (!skip_ifunc))
 	value = ((Elf32_Addr (*) (void)) value) ();
 
       switch (ELF32_R_TYPE (reloc->r_info))
@@ -601,8 +600,8 @@  elf_machine_rela (struct link_map *map, const Elf32_Rela *reloc,
 	    /* This can happen in trace mode if an object could not be
 	       found.  */
 	    break;
-	  if (__builtin_expect (sym->st_size > refsym->st_size, 0)
-	      || (__builtin_expect (sym->st_size < refsym->st_size, 0)
+	  if (__glibc_unlikely (sym->st_size > refsym->st_size)
+	      || (__glibc_unlikely (sym->st_size < refsym->st_size)
 		  && GLRO(dl_verbose)))
 	    {
 	      const char *strtab;
@@ -663,7 +662,8 @@  elf_machine_lazy_rel (struct link_map *map,
   /* Check for unexpected PLT reloc type.  */
   if (__glibc_likely (r_type == R_386_JMP_SLOT))
     {
-      if (__builtin_expect (map->l_mach.plt, 0) == 0)
+      /* Prelinked has been deprecated.  */
+      if (__glibc_likely (map->l_mach.plt == 0))
 	*reloc_addr += l_addr;
       else
 	*reloc_addr = (map->l_mach.plt