diff mbox series

LoongArch: ldconfig: Ignore EF_LARCH_OBJABI_V1 in shared objects

Message ID 20230326111334.9920-1-xry111@xry111.site
State New
Headers show
Series LoongArch: ldconfig: Ignore EF_LARCH_OBJABI_V1 in shared objects | expand

Commit Message

Xi Ruoyao March 26, 2023, 11:13 a.m. UTC
Binutils 2.40 sets EF_LARCH_OBJABI_V1 for shared objects:

    $ ld --version | head -n1
    GNU ld (GNU Binutils) 2.40
    $ echo 'int dummy;' > dummy.c
    $ cc dummy.c -shared
    $ readelf -h a.out | grep Flags
    Flags:                             0x43, DOUBLE-FLOAT, OBJ-v1

We need to ignore it in ldconfig or ldconfig will consider all shared
objects linked by Binutils 2.40 "unsupported".  Maybe we should stop
setting EF_LARCH_OBJABI_V1 for shared objects, but Binutils 2.40 is
already released and we cannot change it.
---
 sysdeps/unix/sysv/linux/loongarch/readelflib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Carlos O'Donell March 27, 2023, 1:44 p.m. UTC | #1
On 3/26/23 07:13, Xi Ruoyao via Libc-alpha wrote:
> Binutils 2.40 sets EF_LARCH_OBJABI_V1 for shared objects:
> 
>     $ ld --version | head -n1
>     GNU ld (GNU Binutils) 2.40
>     $ echo 'int dummy;' > dummy.c
>     $ cc dummy.c -shared
>     $ readelf -h a.out | grep Flags
>     Flags:                             0x43, DOUBLE-FLOAT, OBJ-v1
> 
> We need to ignore it in ldconfig or ldconfig will consider all shared
> objects linked by Binutils 2.40 "unsupported".  Maybe we should stop
> setting EF_LARCH_OBJABI_V1 for shared objects, but Binutils 2.40 is
> already released and we cannot change it.
> ---
>  sysdeps/unix/sysv/linux/loongarch/readelflib.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/loongarch/readelflib.c b/sysdeps/unix/sysv/linux/loongarch/readelflib.c
> index bcaff86b36..ceba355959 100644
> --- a/sysdeps/unix/sysv/linux/loongarch/readelflib.c
> +++ b/sysdeps/unix/sysv/linux/loongarch/readelflib.c
> @@ -40,7 +40,7 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
>  
>    ret = process_elf64_file (file_name, lib, flag, isa_level, soname,
>  				file_contents, file_length);
> -  flags = elf64_header->e_flags;
> +  flags = elf64_header->e_flags & ~EF_LARCH_OBJABI_V1;

Are such objects ABI compatible?

Why isn't this handled below via SUPPORTED_ELF_FLAGS?

>  
>    /* LoongArch linkers encode the floating point ABI as part of the ELF headers.  */
>    switch (flags & SUPPORTED_ELF_FLAGS)
Xi Ruoyao March 27, 2023, 2:57 p.m. UTC | #2
On Mon, 2023-03-27 at 09:44 -0400, Carlos O'Donell wrote:
> On 3/26/23 07:13, Xi Ruoyao via Libc-alpha wrote:
> > Binutils 2.40 sets EF_LARCH_OBJABI_V1 for shared objects:
> > 
> >     $ ld --version | head -n1
> >     GNU ld (GNU Binutils) 2.40
> >     $ echo 'int dummy;' > dummy.c
> >     $ cc dummy.c -shared
> >     $ readelf -h a.out | grep Flags
> >     Flags:                             0x43, DOUBLE-FLOAT, OBJ-v1
> > 
> > We need to ignore it in ldconfig or ldconfig will consider all shared
> > objects linked by Binutils 2.40 "unsupported".  Maybe we should stop
> > setting EF_LARCH_OBJABI_V1 for shared objects, but Binutils 2.40 is
> > already released and we cannot change it.
> > ---
> >  sysdeps/unix/sysv/linux/loongarch/readelflib.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/sysdeps/unix/sysv/linux/loongarch/readelflib.c b/sysdeps/unix/sysv/linux/loongarch/readelflib.c
> > index bcaff86b36..ceba355959 100644
> > --- a/sysdeps/unix/sysv/linux/loongarch/readelflib.c
> > +++ b/sysdeps/unix/sysv/linux/loongarch/readelflib.c
> > @@ -40,7 +40,7 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
> >  
> >    ret = process_elf64_file (file_name, lib, flag, isa_level, soname,
> >                                 file_contents, file_length);
> > -  flags = elf64_header->e_flags;
> > +  flags = elf64_header->e_flags & ~EF_LARCH_OBJABI_V1;
> 
> Are such objects ABI compatible?

This flag was designed for indicating the relocation type usage in a .o
file:

If EF_LARCH_OBJABI_V1 is set, it's allowed to use relocation types with
ID in [64, 100], but it's prohibited to use relocation types with ID in
[22, 46].

If EF_LARCH_OBJABI_V1 is not set, it's allowed to use relocation types
with ID in [22, 46], but it's prohibited to use relocation types with ID
in [64, 100].

A linker may only support the .o files with EF_LARCH_OBJABI_V1 unset
(for example, GNU ld 2.38), or only support the .o files with
EF_LARCH_OBJABI_V1 set (for example, LLD under review at
https://reviews.llvm.org/D138135), or support both (for example, GNU ld
2.40).

If a linker supports both, it's OK to link a EF_LARCH_OBJABI_V1 .o file
together with a non-EF_LARCH_OBJABI_V1 .o file into an executable or
shared object.

But none of relocation type ID in [22, 46] or [64, 100] is runtime
relocation.  I. e. those relocation types should not show up in a shared
object at all (if one shows up, the linker is buggy).  So
EF_LARCH_OBJABI_V1 makes no difference for shared objects. 

> Why isn't this handled below via SUPPORTED_ELF_FLAGS?

If we add this into SUPPORTED_ELF_FLAGS, we'll need

  switch (flags & SUPPORTED_ELF_FLAGS)
    {
      case EF_LARCH_ABI_SOFT_FLOAT:
+     case EF_LARCH_ABI_SOFT_FLOAT | EF_LARCH_OBJABI_V1:
        *flag |= FLAG_LARCH_FLOAT_ABI_SOFT;
       break;
      case EF_LARCH_ABI_DOUBLE_FLOAT:
+     case EF_LARCH_ABI_DOUBLE_FLOAT | EF_LARCH_OBJABI_V1:
        *flag |= FLAG_LARCH_FLOAT_ABI_DOUBLE;
       break;
      default:
        return 1;
    }

I don't think it's better...  But if really you prefer this way I can
send a v2.
Adhemerval Zanella Netto March 27, 2023, 4:54 p.m. UTC | #3
On 27/03/23 11:57, Xi Ruoyao wrote:
> On Mon, 2023-03-27 at 09:44 -0400, Carlos O'Donell wrote:
>> On 3/26/23 07:13, Xi Ruoyao via Libc-alpha wrote:
>>> Binutils 2.40 sets EF_LARCH_OBJABI_V1 for shared objects:
>>>
>>>     $ ld --version | head -n1
>>>     GNU ld (GNU Binutils) 2.40
>>>     $ echo 'int dummy;' > dummy.c
>>>     $ cc dummy.c -shared
>>>     $ readelf -h a.out | grep Flags
>>>     Flags:                             0x43, DOUBLE-FLOAT, OBJ-v1
>>>
>>> We need to ignore it in ldconfig or ldconfig will consider all shared
>>> objects linked by Binutils 2.40 "unsupported".  Maybe we should stop
>>> setting EF_LARCH_OBJABI_V1 for shared objects, but Binutils 2.40 is
>>> already released and we cannot change it.
>>> ---
>>>  sysdeps/unix/sysv/linux/loongarch/readelflib.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/sysdeps/unix/sysv/linux/loongarch/readelflib.c b/sysdeps/unix/sysv/linux/loongarch/readelflib.c
>>> index bcaff86b36..ceba355959 100644
>>> --- a/sysdeps/unix/sysv/linux/loongarch/readelflib.c
>>> +++ b/sysdeps/unix/sysv/linux/loongarch/readelflib.c
>>> @@ -40,7 +40,7 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
>>>  
>>>    ret = process_elf64_file (file_name, lib, flag, isa_level, soname,
>>>                                 file_contents, file_length);
>>> -  flags = elf64_header->e_flags;
>>> +  flags = elf64_header->e_flags & ~EF_LARCH_OBJABI_V1;
>>
>> Are such objects ABI compatible?
> 
> This flag was designed for indicating the relocation type usage in a .o
> file:
> 
> If EF_LARCH_OBJABI_V1 is set, it's allowed to use relocation types with
> ID in [64, 100], but it's prohibited to use relocation types with ID in
> [22, 46].
> 
> If EF_LARCH_OBJABI_V1 is not set, it's allowed to use relocation types
> with ID in [22, 46], but it's prohibited to use relocation types with ID
> in [64, 100].
> 
> A linker may only support the .o files with EF_LARCH_OBJABI_V1 unset
> (for example, GNU ld 2.38), or only support the .o files with
> EF_LARCH_OBJABI_V1 set (for example, LLD under review at
> https://reviews.llvm.org/D138135), or support both (for example, GNU ld
> 2.40).
> 
> If a linker supports both, it's OK to link a EF_LARCH_OBJABI_V1 .o file
> together with a non-EF_LARCH_OBJABI_V1 .o file into an executable or
> shared object.
> 
> But none of relocation type ID in [22, 46] or [64, 100] is runtime
> relocation.  I. e. those relocation types should not show up in a shared
> object at all (if one shows up, the linker is buggy).  So
> EF_LARCH_OBJABI_V1 makes no difference for shared objects. 

Since the boat is already sailed with 2.40, I think the proposed patch
is fine (it would be better if you indeed fix it on 2.41).  I would 
just like to add a comment on why this is required:

  /* The EF_LARCH_OBJABI_V1 flag indicate which set of static relocations
     the object might use and it only considered during static linking, 
     it does not reflect in runtime relocations.  However some binutils 
     version might set it on dynamic shared object, so clear it to avoid 
     see the SO as unsupported.  */ 

> 
>> Why isn't this handled below via SUPPORTED_ELF_FLAGS?
> 
> If we add this into SUPPORTED_ELF_FLAGS, we'll need
> 
>   switch (flags & SUPPORTED_ELF_FLAGS)
>     {
>       case EF_LARCH_ABI_SOFT_FLOAT:
> +     case EF_LARCH_ABI_SOFT_FLOAT | EF_LARCH_OBJABI_V1:
>         *flag |= FLAG_LARCH_FLOAT_ABI_SOFT;
>        break;
>       case EF_LARCH_ABI_DOUBLE_FLOAT:
> +     case EF_LARCH_ABI_DOUBLE_FLOAT | EF_LARCH_OBJABI_V1:
>         *flag |= FLAG_LARCH_FLOAT_ABI_DOUBLE;
>        break;
>       default:
>         return 1;
>     }
> 
> I don't think it's better...  But if really you prefer this way I can
> send a v2.
>
Xi Ruoyao March 27, 2023, 5:46 p.m. UTC | #4
On Mon, 2023-03-27 at 13:54 -0300, Adhemerval Zanella Netto wrote:
> Since the boat is already sailed with 2.40, I think the proposed patch
> is fine (it would be better if you indeed fix it on 2.41).  I would 
> just like to add a comment on why this is required:
> 
>   /* The EF_LARCH_OBJABI_V1 flag indicate which set of static relocations
>      the object might use and it only considered during static linking, 
>      it does not reflect in runtime relocations.  However some binutils 
>      version might set it on dynamic shared object, so clear it to avoid 
>      see the SO as unsupported.  */ 

Looks like Yinyu has already landed the patch.

Yinyu: could you add this comment before the changed line?  I don't have
a write access.
Carlos O'Donell March 29, 2023, 1:12 p.m. UTC | #5
On 3/27/23 12:54, Adhemerval Zanella Netto wrote:
> 
> 
> On 27/03/23 11:57, Xi Ruoyao wrote:
>> On Mon, 2023-03-27 at 09:44 -0400, Carlos O'Donell wrote:
>>> On 3/26/23 07:13, Xi Ruoyao via Libc-alpha wrote:
>>>> Binutils 2.40 sets EF_LARCH_OBJABI_V1 for shared objects:
>>>>
>>>>     $ ld --version | head -n1
>>>>     GNU ld (GNU Binutils) 2.40
>>>>     $ echo 'int dummy;' > dummy.c
>>>>     $ cc dummy.c -shared
>>>>     $ readelf -h a.out | grep Flags
>>>>     Flags:                             0x43, DOUBLE-FLOAT, OBJ-v1
>>>>
>>>> We need to ignore it in ldconfig or ldconfig will consider all shared
>>>> objects linked by Binutils 2.40 "unsupported".  Maybe we should stop
>>>> setting EF_LARCH_OBJABI_V1 for shared objects, but Binutils 2.40 is
>>>> already released and we cannot change it.
>>>> ---
>>>>  sysdeps/unix/sysv/linux/loongarch/readelflib.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/sysdeps/unix/sysv/linux/loongarch/readelflib.c b/sysdeps/unix/sysv/linux/loongarch/readelflib.c
>>>> index bcaff86b36..ceba355959 100644
>>>> --- a/sysdeps/unix/sysv/linux/loongarch/readelflib.c
>>>> +++ b/sysdeps/unix/sysv/linux/loongarch/readelflib.c
>>>> @@ -40,7 +40,7 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
>>>>  
>>>>    ret = process_elf64_file (file_name, lib, flag, isa_level, soname,
>>>>                                 file_contents, file_length);
>>>> -  flags = elf64_header->e_flags;
>>>> +  flags = elf64_header->e_flags & ~EF_LARCH_OBJABI_V1;
>>>
>>> Are such objects ABI compatible?
>>
>> This flag was designed for indicating the relocation type usage in a .o
>> file:

OK, so the final form of the shared objects is compatible, this is only about static
link compatibility and non-shared relocations.

I would suggest reviewing ".gnu_attribute" for an extension to Ehdr->ef_flags that
other architectures use, like ppc64le, to do such compatibility checking and error
detection.

>> If EF_LARCH_OBJABI_V1 is set, it's allowed to use relocation types with
>> ID in [64, 100], but it's prohibited to use relocation types with ID in
>> [22, 46].
>>
>> If EF_LARCH_OBJABI_V1 is not set, it's allowed to use relocation types
>> with ID in [22, 46], but it's prohibited to use relocation types with ID
>> in [64, 100].
>>
>> A linker may only support the .o files with EF_LARCH_OBJABI_V1 unset
>> (for example, GNU ld 2.38), or only support the .o files with
>> EF_LARCH_OBJABI_V1 set (for example, LLD under review at
>> https://reviews.llvm.org/D138135), or support both (for example, GNU ld
>> 2.40).
>>
>> If a linker supports both, it's OK to link a EF_LARCH_OBJABI_V1 .o file
>> together with a non-EF_LARCH_OBJABI_V1 .o file into an executable or
>> shared object.
>>
>> But none of relocation type ID in [22, 46] or [64, 100] is runtime
>> relocation.  I. e. those relocation types should not show up in a shared
>> object at all (if one shows up, the linker is buggy).  So
>> EF_LARCH_OBJABI_V1 makes no difference for shared objects. 
> 
> Since the boat is already sailed with 2.40, I think the proposed patch
> is fine (it would be better if you indeed fix it on 2.41).  I would 
> just like to add a comment on why this is required:
> 
>   /* The EF_LARCH_OBJABI_V1 flag indicate which set of static relocations
>      the object might use and it only considered during static linking, 
>      it does not reflect in runtime relocations.  However some binutils 
>      version might set it on dynamic shared object, so clear it to avoid 
>      see the SO as unsupported.  */ 

Agreed.

commit 07dd75589ecbedec5162a5645d57f8bd093a45db
Author: caiyinyu <caiyinyu@loongson.cn>
Date:   Tue Mar 28 09:19:53 2023 +0800

    LoongArch: ldconfig: Add comments for using EF_LARCH_OBJABI_V1
    
    We added Adhemerval Zanella's comment to explain the reason for
    using EF_LARCH_OBJABI_V1.

And the comment is now in the sources.

This is better, but I have a post-commit review comment here about the overall
usage of ELF flags.

While it is possible to use Ehdr->e_flags for such purposes as marking static
relocation compatibility, you also have the broader use of ".gnu_attribute"
via:

GNU Object Attributes:
https://sourceware.org/binutils/docs/as/GNU-Object-Attributes.html

Which allows a richer set of non-flag-based attributes to be used for object
files and for static link diagnostics.
 -- 
Cheers,
Carlos.
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/loongarch/readelflib.c b/sysdeps/unix/sysv/linux/loongarch/readelflib.c
index bcaff86b36..ceba355959 100644
--- a/sysdeps/unix/sysv/linux/loongarch/readelflib.c
+++ b/sysdeps/unix/sysv/linux/loongarch/readelflib.c
@@ -40,7 +40,7 @@  process_elf_file (const char *file_name, const char *lib, int *flag,
 
   ret = process_elf64_file (file_name, lib, flag, isa_level, soname,
 				file_contents, file_length);
-  flags = elf64_header->e_flags;
+  flags = elf64_header->e_flags & ~EF_LARCH_OBJABI_V1;
 
   /* LoongArch linkers encode the floating point ABI as part of the ELF headers.  */
   switch (flags & SUPPORTED_ELF_FLAGS)