diff mbox series

[v2,RESEND] gdb/elfread.c: Add plt symbol check for _PROCEDURE_LINKAGE_TABLE_

Message ID 20230410123030.25326-1-lihui@loongson.cn
State New
Headers
Series [v2,RESEND] gdb/elfread.c: Add plt symbol check for _PROCEDURE_LINKAGE_TABLE_ |

Commit Message

Hui Li April 10, 2023, 12:30 p.m. UTC 
  In the current code, when execute the following test on LoongArch:

$ make check-gdb TESTS="gdb.base/gnu-ifunc.exp"
 === gdb Summary ===

 # of expected passes           111
 # of unexpected failures       62

According to IFUNC's working process [1]. first time the IFUNC function
is called, the dynamic linker will not simply fill the .got.plt entry
with the actual address of IFUNC symbol, it will call the IFUNC resolver
function and take the return address, uses it as the sym-bound address
and puts it in the .got.plt entry. Initial address in .got.plt entry is
not a real function addresss. Depending on the compiler implementation,
some different addresses will be filled in. Most architectures will use
a .plt entry address to fill in the corresponding .got.plt entry.

In gdb, elf_gnu_ifunc_resolve_addr() will be called to return a real
IFUNC function addresss. First check to see if the real address for
the IFUNC symbol has been resolved by the following function:

elf_gnu_ifunc_resolve_name (const char *name, CORE_ADDR *addr_p)
{
  if (elf_gnu_ifunc_resolve_by_cache (name, addr_p))
    return true;

  if (elf_gnu_ifunc_resolve_by_got (name, addr_p))
    return true;

  return false;
}

in elf_gnu_ifunc_resolve_by_got(), it gets the contents of the
.got.plt entry and determines if the contents is the correct address
by calling elf_gnu_ifunc_record_cache(). Based on the IFUNC working
principle analysis above, the address filled in the .got.plt entry is
not the actual target function address initially, it would be a .plt
entry address corresponding symbol like *@plt. In this case, gdb just
go back to execute the resolver function and puts the return address
in the .got.plt entry. After that, gdb can get a real ifun address via
.got.plt entry.

On LoongArch, initially, each address filled in the .got.plt entries
is the first .plt entry address. Some architectures such as LoongArch
define the symbol _PROCEDURE_LINKAGE_TABLE_ at the start of the .plt
section. This symbol is the first plt entry, so gdb needs to check
this symbol in elf_gnu_ifunc_record_cache().

On LoongArch .got.plt and .plt section as follow:

$objdump -D gdb/testsuite/outputs/gdb.base/gnu-ifunc/gnu-ifunc-0-0-0
...
0000000120010008 <.got.plt>:
   120010008:   ffffffff        0xffffffff
   12001000c:   ffffffff        0xffffffff
        ...
   120010018:   20004000        ll.w            $zero, $zero, 64(0x40)
   12001001c:   00000001        0x00000001
   120010020:   20004000        ll.w            $zero, $zero, 64(0x40)
   120010024:   00000001        0x00000001
   120010028:   20004000        ll.w            $zero, $zero, 64(0x40)
   12001002c:   00000001        0x00000001
   120010030:   20004000        ll.w            $zero, $zero, 64(0x40)
   120010034:   00000001        0x00000001

...
Disassembly of section .plt:

0000000120004000 <_PROCEDURE_LINKAGE_TABLE_>:
   120004000:   1c00018e        pcaddu12i       $t2, 12(0xc)
   120004004:   0011bdad        sub.d           $t1, $t1, $t3
   120004008:   28c021cf        ld.d            $t3, $t2, 8(0x8)
   12000400c:   02ff51ad        addi.d          $t1, $t1, -44(0xfd4)
   120004010:   02c021cc        addi.d          $t0, $t2, 8(0x8)
   120004014:   004505ad        srli.d          $t1, $t1, 0x1
   120004018:   28c0218c        ld.d            $t0, $t0, 8(0x8)
   12000401c:   4c0001e0        jirl            $zero, $t3, 0

0000000120004020 <__libc_start_main@plt>:
   120004020:   1c00018f        pcaddu12i       $t3, 12(0xc)
   120004024:   28ffe1ef        ld.d            $t3, $t3, -8(0xff8)
   120004028:   4c0001ed        jirl            $t1, $t3, 0
   12000402c:   03400000        andi            $zero, $zero, 0x0

0000000120004030 <abort@plt>:
   120004030:   1c00018f        pcaddu12i       $t3, 12(0xc)
   120004034:   28ffc1ef        ld.d            $t3, $t3, -16(0xff0)
   120004038:   4c0001ed        jirl            $t1, $t3, 0
   12000403c:   03400000        andi            $zero, $zero, 0x0

0000000120004040 <gnu_ifunc@plt>:
   120004040:   1c00018f        pcaddu12i       $t3, 12(0xc)
   120004044:   28ffa1ef        ld.d            $t3, $t3, -24(0xfe8)
   120004048:   4c0001ed        jirl            $t1, $t3, 0
   12000404c:   03400000        andi            $zero, $zero, 0x0
...

With this patch:

$make check-gdb TESTS="gdb.base/gnu-ifunc.exp"
=== gdb Summary ===

 #of expected passes            173

[1] https://sourceware.org/glibc/wiki/GNU_IFUNC

Signed-off-by: Hui Li <lihui@loongson.cn>
---
 gdb/elfread.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Hui Li April 27, 2023, 11:44 a.m. UTC | #1 
Are you OK with this change?
Any comments will be much appreciated.

https://sourceware.org/pipermail/gdb-patches/2023-April/198731.html

Thanks,
Hui

On 2023/4/10 下午8:30, Hui Li wrote:
> In the current code, when execute the following test on LoongArch:
> 
> $ make check-gdb TESTS="gdb.base/gnu-ifunc.exp"
>   === gdb Summary ===
> 
>   # of expected passes           111
>   # of unexpected failures       62
> 
> According to IFUNC's working process [1]. first time the IFUNC function
> is called, the dynamic linker will not simply fill the .got.plt entry
> with the actual address of IFUNC symbol, it will call the IFUNC resolver
> function and take the return address, uses it as the sym-bound address
> and puts it in the .got.plt entry. Initial address in .got.plt entry is
> not a real function addresss. Depending on the compiler implementation,
> some different addresses will be filled in. Most architectures will use
> a .plt entry address to fill in the corresponding .got.plt entry.
> 
> In gdb, elf_gnu_ifunc_resolve_addr() will be called to return a real
> IFUNC function addresss. First check to see if the real address for
> the IFUNC symbol has been resolved by the following function:
> 
> elf_gnu_ifunc_resolve_name (const char *name, CORE_ADDR *addr_p)
> {
>    if (elf_gnu_ifunc_resolve_by_cache (name, addr_p))
>      return true;
> 
>    if (elf_gnu_ifunc_resolve_by_got (name, addr_p))
>      return true;
> 
>    return false;
> }
> 
> in elf_gnu_ifunc_resolve_by_got(), it gets the contents of the
> .got.plt entry and determines if the contents is the correct address
> by calling elf_gnu_ifunc_record_cache(). Based on the IFUNC working
> principle analysis above, the address filled in the .got.plt entry is
> not the actual target function address initially, it would be a .plt
> entry address corresponding symbol like *@plt. In this case, gdb just
> go back to execute the resolver function and puts the return address
> in the .got.plt entry. After that, gdb can get a real ifun address via
> .got.plt entry.
> 
> On LoongArch, initially, each address filled in the .got.plt entries
> is the first .plt entry address. Some architectures such as LoongArch
> define the symbol _PROCEDURE_LINKAGE_TABLE_ at the start of the .plt
> section. This symbol is the first plt entry, so gdb needs to check
> this symbol in elf_gnu_ifunc_record_cache().
> 
> On LoongArch .got.plt and .plt section as follow:
> 
> $objdump -D gdb/testsuite/outputs/gdb.base/gnu-ifunc/gnu-ifunc-0-0-0
> ...
> 0000000120010008 <.got.plt>:
>     120010008:   ffffffff        0xffffffff
>     12001000c:   ffffffff        0xffffffff
>          ...
>     120010018:   20004000        ll.w            $zero, $zero, 64(0x40)
>     12001001c:   00000001        0x00000001
>     120010020:   20004000        ll.w            $zero, $zero, 64(0x40)
>     120010024:   00000001        0x00000001
>     120010028:   20004000        ll.w            $zero, $zero, 64(0x40)
>     12001002c:   00000001        0x00000001
>     120010030:   20004000        ll.w            $zero, $zero, 64(0x40)
>     120010034:   00000001        0x00000001
> 
> ...
> Disassembly of section .plt:
> 
> 0000000120004000 <_PROCEDURE_LINKAGE_TABLE_>:
>     120004000:   1c00018e        pcaddu12i       $t2, 12(0xc)
>     120004004:   0011bdad        sub.d           $t1, $t1, $t3
>     120004008:   28c021cf        ld.d            $t3, $t2, 8(0x8)
>     12000400c:   02ff51ad        addi.d          $t1, $t1, -44(0xfd4)
>     120004010:   02c021cc        addi.d          $t0, $t2, 8(0x8)
>     120004014:   004505ad        srli.d          $t1, $t1, 0x1
>     120004018:   28c0218c        ld.d            $t0, $t0, 8(0x8)
>     12000401c:   4c0001e0        jirl            $zero, $t3, 0
> 
> 0000000120004020 <__libc_start_main@plt>:
>     120004020:   1c00018f        pcaddu12i       $t3, 12(0xc)
>     120004024:   28ffe1ef        ld.d            $t3, $t3, -8(0xff8)
>     120004028:   4c0001ed        jirl            $t1, $t3, 0
>     12000402c:   03400000        andi            $zero, $zero, 0x0
> 
> 0000000120004030 <abort@plt>:
>     120004030:   1c00018f        pcaddu12i       $t3, 12(0xc)
>     120004034:   28ffc1ef        ld.d            $t3, $t3, -16(0xff0)
>     120004038:   4c0001ed        jirl            $t1, $t3, 0
>     12000403c:   03400000        andi            $zero, $zero, 0x0
> 
> 0000000120004040 <gnu_ifunc@plt>:
>     120004040:   1c00018f        pcaddu12i       $t3, 12(0xc)
>     120004044:   28ffa1ef        ld.d            $t3, $t3, -24(0xfe8)
>     120004048:   4c0001ed        jirl            $t1, $t3, 0
>     12000404c:   03400000        andi            $zero, $zero, 0x0
> ...
> 
> With this patch:
> 
> $make check-gdb TESTS="gdb.base/gnu-ifunc.exp"
> === gdb Summary ===
> 
>   #of expected passes            173
> 
> [1] https://sourceware.org/glibc/wiki/GNU_IFUNC
> 
> Signed-off-by: Hui Li <lihui@loongson.cn>
> ---
>   gdb/elfread.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index b414da9ed21..1e606783c33 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -722,6 +722,9 @@ elf_gnu_ifunc_record_cache (const char *name, CORE_ADDR addr)
>     if (len > 4 && strcmp (target_name + len - 4, "@plt") == 0)
>       return 0;
>   
> +  if (strcmp (target_name, "_PROCEDURE_LINKAGE_TABLE_") == 0)
> +    return 0;
> +
>     htab = elf_objfile_gnu_ifunc_cache_data.get (objfile);
>     if (htab == NULL)
>       {
>
Tiezhu Yang May 11, 2023, 11:35 a.m. UTC | #2 
On 04/10/2023 08:30 PM, Hui Li wrote:
> In the current code, when execute the following test on LoongArch:
>
> $ make check-gdb TESTS="gdb.base/gnu-ifunc.exp"
>  === gdb Summary ===
>
>  # of expected passes           111
>  # of unexpected failures       62
>
> According to IFUNC's working process [1]. first time the IFUNC function
> is called, the dynamic linker will not simply fill the .got.plt entry
> with the actual address of IFUNC symbol, it will call the IFUNC resolver
> function and take the return address, uses it as the sym-bound address
> and puts it in the .got.plt entry. Initial address in .got.plt entry is
> not a real function addresss. Depending on the compiler implementation,
> some different addresses will be filled in. Most architectures will use
> a .plt entry address to fill in the corresponding .got.plt entry.
>
> In gdb, elf_gnu_ifunc_resolve_addr() will be called to return a real
> IFUNC function addresss. First check to see if the real address for
> the IFUNC symbol has been resolved by the following function:
>
> elf_gnu_ifunc_resolve_name (const char *name, CORE_ADDR *addr_p)
> {
>   if (elf_gnu_ifunc_resolve_by_cache (name, addr_p))
>     return true;
>
>   if (elf_gnu_ifunc_resolve_by_got (name, addr_p))
>     return true;
>
>   return false;
> }
>
> in elf_gnu_ifunc_resolve_by_got(), it gets the contents of the
> .got.plt entry and determines if the contents is the correct address
> by calling elf_gnu_ifunc_record_cache(). Based on the IFUNC working
> principle analysis above, the address filled in the .got.plt entry is
> not the actual target function address initially, it would be a .plt
> entry address corresponding symbol like *@plt. In this case, gdb just
> go back to execute the resolver function and puts the return address
> in the .got.plt entry. After that, gdb can get a real ifun address via
> .got.plt entry.
>
> On LoongArch, initially, each address filled in the .got.plt entries
> is the first .plt entry address. Some architectures such as LoongArch
> define the symbol _PROCEDURE_LINKAGE_TABLE_ at the start of the .plt
> section. This symbol is the first plt entry, so gdb needs to check
> this symbol in elf_gnu_ifunc_record_cache().
>
> On LoongArch .got.plt and .plt section as follow:
>
> $objdump -D gdb/testsuite/outputs/gdb.base/gnu-ifunc/gnu-ifunc-0-0-0
> ...
> 0000000120010008 <.got.plt>:
>    120010008:   ffffffff        0xffffffff
>    12001000c:   ffffffff        0xffffffff
>         ...
>    120010018:   20004000        ll.w            $zero, $zero, 64(0x40)
>    12001001c:   00000001        0x00000001
>    120010020:   20004000        ll.w            $zero, $zero, 64(0x40)
>    120010024:   00000001        0x00000001
>    120010028:   20004000        ll.w            $zero, $zero, 64(0x40)
>    12001002c:   00000001        0x00000001
>    120010030:   20004000        ll.w            $zero, $zero, 64(0x40)
>    120010034:   00000001        0x00000001
>
> ...
> Disassembly of section .plt:
>
> 0000000120004000 <_PROCEDURE_LINKAGE_TABLE_>:
>    120004000:   1c00018e        pcaddu12i       $t2, 12(0xc)
>    120004004:   0011bdad        sub.d           $t1, $t1, $t3
>    120004008:   28c021cf        ld.d            $t3, $t2, 8(0x8)
>    12000400c:   02ff51ad        addi.d          $t1, $t1, -44(0xfd4)
>    120004010:   02c021cc        addi.d          $t0, $t2, 8(0x8)
>    120004014:   004505ad        srli.d          $t1, $t1, 0x1
>    120004018:   28c0218c        ld.d            $t0, $t0, 8(0x8)
>    12000401c:   4c0001e0        jirl            $zero, $t3, 0
>
> 0000000120004020 <__libc_start_main@plt>:
>    120004020:   1c00018f        pcaddu12i       $t3, 12(0xc)
>    120004024:   28ffe1ef        ld.d            $t3, $t3, -8(0xff8)
>    120004028:   4c0001ed        jirl            $t1, $t3, 0
>    12000402c:   03400000        andi            $zero, $zero, 0x0
>
> 0000000120004030 <abort@plt>:
>    120004030:   1c00018f        pcaddu12i       $t3, 12(0xc)
>    120004034:   28ffc1ef        ld.d            $t3, $t3, -16(0xff0)
>    120004038:   4c0001ed        jirl            $t1, $t3, 0
>    12000403c:   03400000        andi            $zero, $zero, 0x0
>
> 0000000120004040 <gnu_ifunc@plt>:
>    120004040:   1c00018f        pcaddu12i       $t3, 12(0xc)
>    120004044:   28ffa1ef        ld.d            $t3, $t3, -24(0xfe8)
>    120004048:   4c0001ed        jirl            $t1, $t3, 0
>    12000404c:   03400000        andi            $zero, $zero, 0x0
> ...
>
> With this patch:
>
> $make check-gdb TESTS="gdb.base/gnu-ifunc.exp"
> === gdb Summary ===
>
>  #of expected passes            173
>
> [1] https://sourceware.org/glibc/wiki/GNU_IFUNC
>
> Signed-off-by: Hui Li <lihui@loongson.cn>
> ---
>  gdb/elfread.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index b414da9ed21..1e606783c33 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -722,6 +722,9 @@ elf_gnu_ifunc_record_cache (const char *name, CORE_ADDR addr)
>    if (len > 4 && strcmp (target_name + len - 4, "@plt") == 0)
>      return 0;
>
> +  if (strcmp (target_name, "_PROCEDURE_LINKAGE_TABLE_") == 0)
> +    return 0;
> +
>    htab = elf_objfile_gnu_ifunc_cache_data.get (objfile);
>    if (htab == NULL)
>      {
>

Hi,

I noticed the following review comments by Tom Tromey [1] on Mar 24:

   It would be helpful to know how precisely things go wrong.
   The patch itself seems reasonable enough -- hacky maybe but not out of
   the ordinary way -- but I don't understand how it relates to the problem.
   Like, why does ignoring this symbol here affect the results?

Are you OK for this v2 patch with the updated commit message resent on
Apr 10 [2]? If no more comments, let me push this patch next week.

[1] https://sourceware.org/pipermail/gdb-patches/2023-March/198285.html
[2] https://sourceware.org/pipermail/gdb-patches/2023-April/198731.html

Thanks,
Tiezhu
Tiezhu Yang May 18, 2023, 2:38 p.m. UTC | #3 
On 5/11/23 19:35, Tiezhu Yang wrote:
> 
> 
> On 04/10/2023 08:30 PM, Hui Li wrote:
>> In the current code, when execute the following test on LoongArch:
>>
>> $ make check-gdb TESTS="gdb.base/gnu-ifunc.exp"
>>  === gdb Summary ===
>>
>>  # of expected passes           111
>>  # of unexpected failures       62
>>
...
>> With this patch:
>>
>> $make check-gdb TESTS="gdb.base/gnu-ifunc.exp"
>> === gdb Summary ===
>>
>>  #of expected passes            173
>>
>> [1] https://sourceware.org/glibc/wiki/GNU_IFUNC
>>
>> Signed-off-by: Hui Li <lihui@loongson.cn>
>> ---
>>  gdb/elfread.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/gdb/elfread.c b/gdb/elfread.c
>> index b414da9ed21..1e606783c33 100644
>> --- a/gdb/elfread.c
>> +++ b/gdb/elfread.c
>> @@ -722,6 +722,9 @@ elf_gnu_ifunc_record_cache (const char *name, 
>> CORE_ADDR addr)
>>    if (len > 4 && strcmp (target_name + len - 4, "@plt") == 0)
>>      return 0;
>>
>> +  if (strcmp (target_name, "_PROCEDURE_LINKAGE_TABLE_") == 0)
>> +    return 0;
>> +
>>    htab = elf_objfile_gnu_ifunc_cache_data.get (objfile);
>>    if (htab == NULL)
>>      {
>>
> 
> Hi,
> 
> I noticed the following review comments by Tom Tromey [1] on Mar 24:
> 
>    It would be helpful to know how precisely things go wrong.
>    The patch itself seems reasonable enough -- hacky maybe but not out of
>    the ordinary way -- but I don't understand how it relates to the 
> problem.
>    Like, why does ignoring this symbol here affect the results?
> 
> Are you OK for this v2 patch with the updated commit message resent on
> Apr 10 [2]? If no more comments, let me push this patch next week.
> 
> [1] https://sourceware.org/pipermail/gdb-patches/2023-March/198285.html
> [2] https://sourceware.org/pipermail/gdb-patches/2023-April/198731.html
> 

This patch is mainly related with LoongArch and no side effects
on the other archs, pushed.

Thanks,
Tiezhu
Tom Tromey May 18, 2023, 6:01 p.m. UTC | #4 
>>>>> Tiezhu Yang <yangtiezhu@loongson.cn> writes:

> This patch is mainly related with LoongArch and no side effects
> on the other archs, pushed.

Please don't do this in the future.  It's not ok to touch generic code
like this under the aegis of supporting some random port.

Tom
Tiezhu Yang May 19, 2023, 1:34 a.m. UTC | #5 
On 05/19/2023 02:01 AM, Tom Tromey wrote:
>>>>>> Tiezhu Yang <yangtiezhu@loongson.cn> writes:
>
>> This patch is mainly related with LoongArch and no side effects
>> on the other archs, pushed.
>
> Please don't do this in the future.  It's not ok to touch generic code
> like this under the aegis of supporting some random port.
>
> Tom
>

OK, sorry for that. For generic code, Reviewed-By or Approved-By
is necessary before pushing, I will be careful and wait more time
to receive the tags, thank you.

Thanks,
Tiezhu
diff mbox series

Patch

diff --git a/gdb/elfread.c b/gdb/elfread.c
index b414da9ed21..1e606783c33 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -722,6 +722,9 @@  elf_gnu_ifunc_record_cache (const char *name, CORE_ADDR addr)
   if (len > 4 && strcmp (target_name + len - 4, "@plt") == 0)
     return 0;
 
+  if (strcmp (target_name, "_PROCEDURE_LINKAGE_TABLE_") == 0)
+    return 0;
+
   htab = elf_objfile_gnu_ifunc_cache_data.get (objfile);
   if (htab == NULL)
     {