diff mbox series

LoongArch: Fix ABI related macros in elf.h to keep consistent with binutils[1].

Message ID 20221031034334.3027740-1-caiyinyu@loongson.cn
State New
Headers show
Series LoongArch: Fix ABI related macros in elf.h to keep consistent with binutils[1]. | expand

Commit Message

caiyinyu Oct. 31, 2022, 3:43 a.m. UTC 
[1]:
https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=c4a7e6b56218e1d5a858682186b542e2eae01a4a;hp=0d94a8735055432029237612a6eb9165db1ec9dd
[2]:
Reference: https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html#_e_flags_identifies_abi_type_and_version
---
 elf/elf.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Xi Ruoyao Oct. 31, 2022, 6:34 a.m. UTC | #1 
On Mon, 2022-10-31 at 11:43 +0800, caiyinyu wrote:
> [1]:
> https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=c4a7e6b56218e1d5a858682186b542e2eae01a4a;hp=0d94a8735055432029237612a6eb9165db1ec9dd
> [2]:
> Reference: https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html#_e_flags_identifies_abi_type_and_version
> ---
>  elf/elf.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/elf/elf.h b/elf/elf.h
> index 3f08823a30..383eb5fbff 100644
> --- a/elf/elf.h
> +++ b/elf/elf.h
> @@ -4093,8 +4093,10 @@ enum
>  #define R_NDS32_TLS_DESC       119
>  
>  /* LoongArch ELF Flags */
> -#define EF_LARCH_ABI           0x07
> -#define EF_LARCH_ABI_LP64D     0x03

Does Glibc API policy allow to remove a macro definition in a public
header?

> +#define EF_LOONGARCH_ABI_SOFT_FLOAT     0x1
> +#define EF_LOONGARCH_ABI_SINGLE_FLOAT   0x2
> +#define EF_LOONGARCH_ABI_DOUBLE_FLOAT   0x3
> +#define EF_LOONGARCH_OBJABI_V1          0x40
>  
>  /* LoongArch specific dynamic relocations */
>  #define R_LARCH_NONE           0
Carlos O'Donell Oct. 31, 2022, 1:12 p.m. UTC | #2 
On 10/31/22 02:34, Xi Ruoyao via Libc-alpha wrote:
> On Mon, 2022-10-31 at 11:43 +0800, caiyinyu wrote:
>> [1]:
>> https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=c4a7e6b56218e1d5a858682186b542e2eae01a4a;hp=0d94a8735055432029237612a6eb9165db1ec9dd
>> [2]:
>> Reference: https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html#_e_flags_identifies_abi_type_and_version
>> ---
>>  elf/elf.h | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/elf/elf.h b/elf/elf.h
>> index 3f08823a30..383eb5fbff 100644
>> --- a/elf/elf.h
>> +++ b/elf/elf.h
>> @@ -4093,8 +4093,10 @@ enum
>>  #define R_NDS32_TLS_DESC       119
>>  
>>  /* LoongArch ELF Flags */
>> -#define EF_LARCH_ABI           0x07

I assume this is a mask for the bits.

>> -#define EF_LARCH_ABI_LP64D     0x03
> 
> Does Glibc API policy allow to remove a macro definition in a public
> header?

If a public glibc release has been made of this header then it is public ABI.

How big is the LoongArch community? You could remove these things if you wanted,
but it is not good for existing source compatibility.
 
>> +#define EF_LOONGARCH_ABI_SOFT_FLOAT     0x1
>> +#define EF_LOONGARCH_ABI_SINGLE_FLOAT   0x2
>> +#define EF_LOONGARCH_ABI_DOUBLE_FLOAT   0x3

Best practice is to keep the old macro names around.

Add the new macro names.

The same value should have the *same* semantics, otherwise it's an ABI break.

>> +#define EF_LOONGARCH_OBJABI_V1          0x40
>>  
>>  /* LoongArch specific dynamic relocations */
>>  #define R_LARCH_NONE           0
>
Xi Ruoyao Nov. 1, 2022, 2 a.m. UTC | #3 
On Mon, 2022-10-31 at 09:12 -0400, Carlos O'Donell wrote:
> On 10/31/22 02:34, Xi Ruoyao via Libc-alpha wrote:
> > On Mon, 2022-10-31 at 11:43 +0800, caiyinyu wrote:
> > > [1]:
> > > https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=c4a7e6b56218e1d5a858682186b542e2eae01a4a;hp=0d94a8735055432029237612a6eb9165db1ec9dd
> > > [2]:
> > > Reference: https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html#_e_flags_identifies_abi_type_and_version
> > > ---
> > >  elf/elf.h | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/elf/elf.h b/elf/elf.h
> > > index 3f08823a30..383eb5fbff 100644
> > > --- a/elf/elf.h
> > > +++ b/elf/elf.h
> > > @@ -4093,8 +4093,10 @@ enum
> > >  #define R_NDS32_TLS_DESC       119
> > >  
> > >  /* LoongArch ELF Flags */
> > > -#define EF_LARCH_ABI           0x07
> 
> I assume this is a mask for the bits.
> 
> > > -#define EF_LARCH_ABI_LP64D     0x03
> > 
> > Does Glibc API policy allow to remove a macro definition in a public
> > header?
> 
> If a public glibc release has been made of this header then it is public ABI.
> 
> How big is the LoongArch community? You could remove these things if you wanted,
> but it is not good for existing source compatibility.
>  
> > > +#define EF_LOONGARCH_ABI_SOFT_FLOAT     0x1
> > > +#define EF_LOONGARCH_ABI_SINGLE_FLOAT   0x2
> > > +#define EF_LOONGARCH_ABI_DOUBLE_FLOAT   0x3

Hmm, is "LARCH" -> "LOONGARCH" change intentional here?

> Best practice is to keep the old macro names around.
> 
> Add the new macro names.

I agree.
caiyinyu Nov. 1, 2022, 2:02 a.m. UTC | #4 
在 2022/10/31 下午9:12, Carlos O'Donell 写道:
> On 10/31/22 02:34, Xi Ruoyao via Libc-alpha wrote:
>> On Mon, 2022-10-31 at 11:43 +0800, caiyinyu wrote:
>>> [1]:
>>> https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=c4a7e6b56218e1d5a858682186b542e2eae01a4a;hp=0d94a8735055432029237612a6eb9165db1ec9dd
>>> [2]:
>>> Reference: https://loongson.github.io/LoongArch-Documentation/LoongArch-ELF-ABI-EN.html#_e_flags_identifies_abi_type_and_version
>>> ---
>>>   elf/elf.h | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/elf/elf.h b/elf/elf.h
>>> index 3f08823a30..383eb5fbff 100644
>>> --- a/elf/elf.h
>>> +++ b/elf/elf.h
>>> @@ -4093,8 +4093,10 @@ enum
>>>   #define R_NDS32_TLS_DESC       119
>>>   
>>>   /* LoongArch ELF Flags */
>>> -#define EF_LARCH_ABI           0x07
> I assume this is a mask for the bits.
>
>>> -#define EF_LARCH_ABI_LP64D     0x03
>> Does Glibc API policy allow to remove a macro definition in a public
>> header?
> If a public glibc release has been made of this header then it is public ABI.
>
> How big is the LoongArch community? You could remove these things if you wanted,
> but it is not good for existing source compatibility.
>   
>>> +#define EF_LOONGARCH_ABI_SOFT_FLOAT     0x1
>>> +#define EF_LOONGARCH_ABI_SINGLE_FLOAT   0x2
>>> +#define EF_LOONGARCH_ABI_DOUBLE_FLOAT   0x3
> Best practice is to keep the old macro names around.
>
> Add the new macro names.
>
> The same value should have the *same* semantics, otherwise it's an ABI break.

Fixed.


diff --git a/elf/elf.h b/elf/elf.h

index 3f08823a30..d51eb75a73 100644
--- a/elf/elf.h
+++ b/elf/elf.h
@@ -4093,8 +4093,11 @@ enum
  #define R_NDS32_TLS_DESC       119

  /* LoongArch ELF Flags */
-#define EF_LARCH_ABI           0x07
-#define EF_LARCH_ABI_LP64D     0x03
+#define EF_LOONGARCH_ABI_MODIFIER_MASK 0x07
+#define EF_LOONGARCH_ABI_SOFT_FLOAT     0x01
+#define EF_LOONGARCH_ABI_SINGLE_FLOAT   0x02
+#define EF_LOONGARCH_ABI_DOUBLE_FLOAT   0x03
+#define EF_LOONGARCH_OBJABI_V1          0x40

  /* LoongArch specific dynamic relocations */
  #define R_LARCH_NONE           0

>>> +#define EF_LOONGARCH_OBJABI_V1          0x40
>>>   
>>>   /* LoongArch specific dynamic relocations */
>>>   #define R_LARCH_NONE           0
Xi Ruoyao Nov. 1, 2022, 2:14 a.m. UTC | #5 
On Tue, 2022-11-01 at 10:02 +0800, caiyinyu wrote:

> diff --git a/elf/elf.h b/elf/elf.h
> 
> index 3f08823a30..d51eb75a73 100644
> --- a/elf/elf.h
> +++ b/elf/elf.h
> @@ -4093,8 +4093,11 @@ enum
>   #define R_NDS32_TLS_DESC       119
> 
>   /* LoongArch ELF Flags */
> -#define EF_LARCH_ABI           0x07
> -#define EF_LARCH_ABI_LP64D     0x03
> +#define EF_LOONGARCH_ABI_MODIFIER_MASK 0x07
> +#define EF_LOONGARCH_ABI_SOFT_FLOAT     0x01
> +#define EF_LOONGARCH_ABI_SINGLE_FLOAT   0x02
> +#define EF_LOONGARCH_ABI_DOUBLE_FLOAT   0x03
> +#define EF_LOONGARCH_OBJABI_V1          0x40

Again, is "LARCH" -> "LOONGARCH" change intentional?  The ABI doc does
not specified the name of these macros...  And we already have R_LARCH_*
so the difference of "LARCH" and "LOONGARCH" seems puzzling.

And is it an option to keep old names there with a warning?

#define EF_LARCH_ABI_LP64D __glibc_macro_warning ("EF_LARCH_ABI_LP64D is deprecated") 0x03
caiyinyu Nov. 2, 2022, 6:13 a.m. UTC | #6 
在 2022/11/1 上午10:14, Xi Ruoyao 写道:
> On Tue, 2022-11-01 at 10:02 +0800, caiyinyu wrote:
>
>> diff --git a/elf/elf.h b/elf/elf.h
>>
>> index 3f08823a30..d51eb75a73 100644
>> --- a/elf/elf.h
>> +++ b/elf/elf.h
>> @@ -4093,8 +4093,11 @@ enum
>>    #define R_NDS32_TLS_DESC       119
>>
>>    /* LoongArch ELF Flags */
>> -#define EF_LARCH_ABI           0x07
>> -#define EF_LARCH_ABI_LP64D     0x03
>> +#define EF_LOONGARCH_ABI_MODIFIER_MASK 0x07
>> +#define EF_LOONGARCH_ABI_SOFT_FLOAT     0x01
>> +#define EF_LOONGARCH_ABI_SINGLE_FLOAT   0x02
>> +#define EF_LOONGARCH_ABI_DOUBLE_FLOAT   0x03
>> +#define EF_LOONGARCH_OBJABI_V1          0x40
> Again, is "LARCH" -> "LOONGARCH" change intentional?  The ABI doc does
> not specified the name of these macros...  And we already have R_LARCH_*
> so the difference of "LARCH" and "LOONGARCH" seems puzzling.

We will use *LARCH* to keep consistent in glibc and binutils like other 
arches

and update the docs soon:

diff --git a/elf/elf.h b/elf/elf.h
index d51eb75a73..04dc2ee563 100644
--- a/elf/elf.h
+++ b/elf/elf.h
@@ -4093,11 +4093,11 @@ enum
  #define R_NDS32_TLS_DESC       119

  /* LoongArch ELF Flags */
-#define EF_LOONGARCH_ABI_MODIFIER_MASK 0x07
-#define EF_LOONGARCH_ABI_SOFT_FLOAT     0x01
-#define EF_LOONGARCH_ABI_SINGLE_FLOAT   0x02
-#define EF_LOONGARCH_ABI_DOUBLE_FLOAT   0x03
-#define EF_LOONGARCH_OBJABI_V1          0x40
+#define EF_LARCH_ABI_MODIFIER_MASK  0x07
+#define EF_LARCH_ABI_SOFT_FLOAT     0x01
+#define EF_LARCH_ABI_SINGLE_FLOAT   0x02
+#define EF_LARCH_ABI_DOUBLE_FLOAT   0x03
+#define EF_LARCH_OBJABI_V1          0x40

  /* LoongArch specific dynamic relocations */
  #define R_LARCH_NONE           0


> And is it an option to keep old names there with a warning?
>
> #define EF_LARCH_ABI_LP64D __glibc_macro_warning ("EF_LARCH_ABI_LP64D is deprecated") 0x03
>
I tend to remove it now.
WANG Xuerui Nov. 2, 2022, 6:19 a.m. UTC | #7 
On 2022/11/2 14:13, caiyinyu wrote:
>
> 在 2022/11/1 上午10:14, Xi Ruoyao 写道:
>> On Tue, 2022-11-01 at 10:02 +0800, caiyinyu wrote:
>>
>>> diff --git a/elf/elf.h b/elf/elf.h
>>>
>>> index 3f08823a30..d51eb75a73 100644
>>> --- a/elf/elf.h
>>> +++ b/elf/elf.h
>>> @@ -4093,8 +4093,11 @@ enum
>>>    #define R_NDS32_TLS_DESC       119
>>>
>>>    /* LoongArch ELF Flags */
>>> -#define EF_LARCH_ABI           0x07
>>> -#define EF_LARCH_ABI_LP64D     0x03
>>> +#define EF_LOONGARCH_ABI_MODIFIER_MASK 0x07
>>> +#define EF_LOONGARCH_ABI_SOFT_FLOAT     0x01
>>> +#define EF_LOONGARCH_ABI_SINGLE_FLOAT   0x02
>>> +#define EF_LOONGARCH_ABI_DOUBLE_FLOAT   0x03
>>> +#define EF_LOONGARCH_OBJABI_V1          0x40
>> Again, is "LARCH" -> "LOONGARCH" change intentional?  The ABI doc does
>> not specified the name of these macros...  And we already have R_LARCH_*
>> so the difference of "LARCH" and "LOONGARCH" seems puzzling.
>
> We will use *LARCH* to keep consistent in glibc and binutils like 
> other arches

Unfortunately it's EF_LOONGARCH_xxx in binutils from day one [1]... BTW 
the inconsistency is also present from day one.

[1]: 
https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=include/elf/loongarch.h;h=a6341b46791d3f820f6e120cdc82ca30020ca7ab;hb=HEAD

>
>> And is it an option to keep old names there with a warning?
>>
>> #define EF_LARCH_ABI_LP64D __glibc_macro_warning ("EF_LARCH_ABI_LP64D 
>> is deprecated") 0x03
>>
> I tend to remove it now.
>
While there's very few LoongArch users running fully open-source systems 
(most of them instead running the so-called old world distros which are 
all more or less commercial distros), deprecating for one release might 
be better, although just removing wouldn't cause much harm either. The 
current user base of "new world" LoongArch distros are mostly devs or 
power users so they know what to expect.
Xi Ruoyao Nov. 2, 2022, 10:41 a.m. UTC | #8 
On Wed, 2022-11-02 at 14:19 +0800, WANG Xuerui wrote:
> 
> On 2022/11/2 14:13, caiyinyu wrote:
> > 
> > 在 2022/11/1 上午10:14, Xi Ruoyao 写道:
> > > On Tue, 2022-11-01 at 10:02 +0800, caiyinyu wrote:
> > > 
> > > > diff --git a/elf/elf.h b/elf/elf.h
> > > > 
> > > > index 3f08823a30..d51eb75a73 100644
> > > > --- a/elf/elf.h
> > > > +++ b/elf/elf.h
> > > > @@ -4093,8 +4093,11 @@ enum
> > > >    #define R_NDS32_TLS_DESC       119
> > > > 
> > > >    /* LoongArch ELF Flags */
> > > > -#define EF_LARCH_ABI           0x07
> > > > -#define EF_LARCH_ABI_LP64D     0x03
> > > > +#define EF_LOONGARCH_ABI_MODIFIER_MASK 0x07
> > > > +#define EF_LOONGARCH_ABI_SOFT_FLOAT     0x01
> > > > +#define EF_LOONGARCH_ABI_SINGLE_FLOAT   0x02
> > > > +#define EF_LOONGARCH_ABI_DOUBLE_FLOAT   0x03
> > > > +#define EF_LOONGARCH_OBJABI_V1          0x40
> > > Again, is "LARCH" -> "LOONGARCH" change intentional?  The ABI doc does
> > > not specified the name of these macros...  And we already have R_LARCH_*
> > > so the difference of "LARCH" and "LOONGARCH" seems puzzling.
> > 
> > We will use *LARCH* to keep consistent in glibc and binutils like 
> > other arches
> 
> Unfortunately it's EF_LOONGARCH_xxx in binutils from day one [1]... BTW 
> the inconsistency is also present from day one.
> 
> [1]: 
> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=include/elf/loongarch.h;h=a6341b46791d3f820f6e120cdc82ca30020ca7ab;hb=HEAD

Fortunately EF_LOONGARCH_xxx in binutils are not a part of the public
API of libbfd (i. e. include/elf/loongarch.h is not installed).  So I
think we should use EF_LARCH_* instead of EF_LOONGARCH_* in every public
interface.  And we can "fix" binutils later.

> > > And is it an option to keep old names there with a warning?
> > > 
> > > #define EF_LARCH_ABI_LP64D __glibc_macro_warning ("EF_LARCH_ABI_LP64D 
> > > is deprecated") 0x03
> > > 
> > I tend to remove it now.
> > 
> While there's very few LoongArch users running fully open-source systems 
> (most of them instead running the so-called old world distros which are 
> all more or less commercial distros), deprecating for one release might 
> be better, although just removing wouldn't cause much harm either. The
> current user base of "new world" LoongArch distros are mostly devs or 
> power users so they know what to expect.

I don't have a strong opinion.  If you insist to remove it now I won't
object.
diff mbox series

Patch

diff --git a/elf/elf.h b/elf/elf.h
index 3f08823a30..383eb5fbff 100644
--- a/elf/elf.h
+++ b/elf/elf.h
@@ -4093,8 +4093,10 @@  enum
 #define R_NDS32_TLS_DESC	119
 
 /* LoongArch ELF Flags */
-#define EF_LARCH_ABI    	0x07
-#define EF_LARCH_ABI_LP64D	0x03
+#define EF_LOONGARCH_ABI_SOFT_FLOAT     0x1
+#define EF_LOONGARCH_ABI_SINGLE_FLOAT   0x2
+#define EF_LOONGARCH_ABI_DOUBLE_FLOAT   0x3
+#define EF_LOONGARCH_OBJABI_V1          0x40
 
 /* LoongArch specific dynamic relocations */
 #define R_LARCH_NONE		0