Patchwork [v2,2/2] libc-abis: Define ABSOLUTE ABI [BZ #19818][BZ #23307]

login
register
mail settings
Submitter Maciej W. Rozycki
Date July 5, 2018, 3:41 p.m.
Message ID <alpine.DEB.2.00.1807051517290.22261@tp.orcam.me.uk>
Download mbox | patch
Permalink /patch/28243/
State New
Headers show

Comments

Maciej W. Rozycki - July 5, 2018, 3:41 p.m.
Define a new ABSOLUTE ABI for static linker's use with EI_ABIVERSION 
where correct absolute (SHN_ABS) symbol run-time load semantics is 
required.  This way it can be ensured at static link time that a program 
or DSO will not suffer from previous semantics where absolute symbols 
were relocated by the base address, or symbols whose `st_value' is zero 
silently ignored leading to a confusing "undefined symbol" error message 
at load time, and instead "ELF file ABI version invalid" is printed with 
old dynamic loaders, making it clear that there is an ABI version 
incompatibility.

	[BZ #19818]
	[BZ #23307]
	* libc-abis (ABSOLUTE): New ABI.
	* sysdeps/unix/sysv/linux/mips/libc-abis (ABSOLUTE): New ABI.
	* NEWS: Mention the new ABI.
---
Hi Carlos,

> >  If so, then I think a NEWS entry will be due, such as:
> > 
> > * The maximum libc ABI supported has been increased to reflect absolute
> >   symbol support.  This can be used by the static linker in the ELF file
> >   header's EI_ABIVERSION field to indicate such support is required.
> 
> Suggest:
> 
> * The GNU C Library now has correct support for ABSOLUTE symbols
>   (SHN_ABS-relative symbols).  Previously such ABSOLUTE symbols were relocated
>   inconsistently or wrongly in some cases.  The GNU linker has been enhanced to
>   fix this issue, but it must communicate these newer semantics to the dynamic
>   loader by setting the ELF file's identification (EI_ABIVERSION field) to 
>   indicate such support is required.

 I think it's a bit inaccurate.

 First of all the fix to binutils PR ld/21375 is only related to our BZ 
#19818 and BZ #23307 in that it makes use of the feature that fixes to our 
BZs make work.  So I don't think we can call the fix to PR ld/21375 a 
linker enhancement addressing (a linker's part of) the issue we have.  
Absolute symbols have always worked with the GNU linker (although there 
indeed used to be odd corner cases which have since been addressed and 
the `LD_FEATURE ("SANE_EXPR")' linker-script command added in the course).

 And then I think we can more accurately express what issues we had with 
absolute symbols without losing comprehensibility of the release note to 
its addressees.

 I've included the proposed text in the updated patch carried with this 
message.  WDYT?

> > which I will include in the actual commit.
> 
> This looks good to me.
> 
> I don't see an easy way around this problem.
> 
> I think bumping EI_ABIVERSION is the best cost/value solution there.

 Thank you for your review and assessment of the situation.

 As a side note (prompted by how you phrased the release note), neither 
the fixes to BZ #19818 and BZ #23307 nor (obviously) this change retains 
the old semantics for binaries that carry a lower value in EI_ABIVERSION 
(and we actually don't do that for any previously defined EI_ABIVERSION 
values either).

 I think the chance that a piece of software relies on the broken previous 
(lack of) interpretation of absolute symbols is very slim -- after all 
what would be the purpose of going through all the arrangement required to 
get an absolute dynamic symbol produced in an ELF file only to have it 
then interpreted, in a non-compliant way, as far as the ELF gABI is 
concerned, as an ordinary relocated one?

 So I think it is fine that we consider absolute symbols previously 
unsupported and do not provide backwards-compatible semantics for ELF 
files carrying a previously defined EI_ABIVERSION value.  Having such 
support would require engineering effort to conditionalise code at run 
time on the value of EI_ABIVERSION, which would also affect performance 
(albeit only for absolute symbols, so likely not significantly enough for 
that to be a real concern), and then only to support non-compliant code 
that may not even exist.

 If such code does surface, then we can assess the situation again, and 
decide what to do about that.

 For the record, in the course of fixing an unrelated issue with section 
garbage collection in the GNU linker (binutils commit 12f09816cecc 
("MIPS/BFD: Make section GC work with `ict_irix5' targets")), I have come 
across these symbols produced for use by the IRIX 5 dynamic loader 
(according to `readelf'):

Symbol table '\.dynsym' contains 4 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 00000000     0 SECTION GLOBAL PROTECTED  ABS _procedure_table_size
     2: 00000000     0 SECTION GLOBAL PROTECTED PRC [0xff02] _procedure_string_table
     3: 00000000     0 SECTION GLOBAL PROTECTED PRC [0xff02] _procedure_table

which clearly indicates how at least one non-GNU dynamic loader interprets 
absolute symbols (the size of the procedure table, held in 
`_procedure_table_size', surely is not supposed to be relocated by the 
base address); for the record, `PRC [0xff02]' denotes the special 
SHN_MIPS_TEXT section used to hold code in the MIPS/IRIX psABI in place of 
the usual `.text' section.

 OK for this change with updated NEWS then?

  Maciej

Changes from v1:

- NEWS entry added.
---
 NEWS                                   |    7 +++++++
 libc-abis                              |    2 ++
 sysdeps/unix/sysv/linux/mips/libc-abis |    2 ++
 3 files changed, 11 insertions(+)

glibc-abi-absolute.diff
Carlos O'Donell - July 5, 2018, 4:28 p.m.
On 07/05/2018 11:41 AM, Maciej W. Rozycki wrote:
> Define a new ABSOLUTE ABI for static linker's use with EI_ABIVERSION 
> where correct absolute (SHN_ABS) symbol run-time load semantics is 
> required.  This way it can be ensured at static link time that a program 
> or DSO will not suffer from previous semantics where absolute symbols 
> were relocated by the base address, or symbols whose `st_value' is zero 
> silently ignored leading to a confusing "undefined symbol" error message 
> at load time, and instead "ELF file ABI version invalid" is printed with 
> old dynamic loaders, making it clear that there is an ABI version 
> incompatibility.

OK.

> 	[BZ #19818]
> 	[BZ #23307]
> 	* libc-abis (ABSOLUTE): New ABI.
> 	* sysdeps/unix/sysv/linux/mips/libc-abis (ABSOLUTE): New ABI.
> 	* NEWS: Mention the new ABI.
> ---
> Hi Carlos,
> 
>>>  If so, then I think a NEWS entry will be due, such as:
>>>
>>> * The maximum libc ABI supported has been increased to reflect absolute
>>>   symbol support.  This can be used by the static linker in the ELF file
>>>   header's EI_ABIVERSION field to indicate such support is required.
>>
>> Suggest:
>>
>> * The GNU C Library now has correct support for ABSOLUTE symbols
>>   (SHN_ABS-relative symbols).  Previously such ABSOLUTE symbols were relocated
>>   inconsistently or wrongly in some cases.  The GNU linker has been enhanced to
>>   fix this issue, but it must communicate these newer semantics to the dynamic
>>   loader by setting the ELF file's identification (EI_ABIVERSION field) to 
>>   indicate such support is required.
> 
>  I think it's a bit inaccurate.
> 
>  First of all the fix to binutils PR ld/21375 is only related to our BZ 
> #19818 and BZ #23307 in that it makes use of the feature that fixes to our 
> BZs make work.  So I don't think we can call the fix to PR ld/21375 a 
> linker enhancement addressing (a linker's part of) the issue we have.  
> Absolute symbols have always worked with the GNU linker (although there 
> indeed used to be odd corner cases which have since been addressed and 
> the `LD_FEATURE ("SANE_EXPR")' linker-script command added in the course).

Sure, that sounds OK to me.

>  And then I think we can more accurately express what issues we had with 
> absolute symbols without losing comprehensibility of the release note to 
> its addressees.
> 
>  I've included the proposed text in the updated patch carried with this 
> message.  WDYT?

Looks good with a minor change.

>>> which I will include in the actual commit.
>>
>> This looks good to me.
>>
>> I don't see an easy way around this problem.
>>
>> I think bumping EI_ABIVERSION is the best cost/value solution there.
> 
>  Thank you for your review and assessment of the situation.
> 
>  As a side note (prompted by how you phrased the release note), neither 
> the fixes to BZ #19818 and BZ #23307 nor (obviously) this change retains 
> the old semantics for binaries that carry a lower value in EI_ABIVERSION 
> (and we actually don't do that for any previously defined EI_ABIVERSION 
> values either).

OK.

>  I think the chance that a piece of software relies on the broken previous 
> (lack of) interpretation of absolute symbols is very slim -- after all 
> what would be the purpose of going through all the arrangement required to 
> get an absolute dynamic symbol produced in an ELF file only to have it 
> then interpreted, in a non-compliant way, as far as the ELF gABI is 
> concerned, as an ordinary relocated one?

Agreed.

In the case where SHN_ABS-relative symbols could not have possible worked
as intended there is no reason to have backwards compatibility.

Similarly we had ELF gABI issues parsing DSTs and I didn't keep the old
parsing code, I just updated it to sensibly meet the requirements of the
gABI. Old code would have been really convoluted to have wanted untranslated
DSTs show up in their output (it would have to have been some kind of auto
generated filename with $ and { and } in the name, and we've never seen
that).

>  So I think it is fine that we consider absolute symbols previously 
> unsupported and do not provide backwards-compatible semantics for ELF 
> files carrying a previously defined EI_ABIVERSION value.  Having such 
> support would require engineering effort to conditionalise code at run 
> time on the value of EI_ABIVERSION, which would also affect performance 
> (albeit only for absolute symbols, so likely not significantly enough for 
> that to be a real concern), and then only to support non-compliant code 
> that may not even exist.

Correct. I agree with your position.

>  If such code does surface, then we can assess the situation again, and 
> decide what to do about that.

Correct. We can still make that change in theory. Binaries without the
new EI_ABIVERSION can be handled differently.

>  For the record, in the course of fixing an unrelated issue with section 
> garbage collection in the GNU linker (binutils commit 12f09816cecc 
> ("MIPS/BFD: Make section GC work with `ict_irix5' targets")), I have come 
> across these symbols produced for use by the IRIX 5 dynamic loader 
> (according to `readelf'):
> 
> Symbol table '\.dynsym' contains 4 entries:
>    Num:    Value  Size Type    Bind   Vis      Ndx Name
>      0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND
>      1: 00000000     0 SECTION GLOBAL PROTECTED  ABS _procedure_table_size
>      2: 00000000     0 SECTION GLOBAL PROTECTED PRC [0xff02] _procedure_string_table
>      3: 00000000     0 SECTION GLOBAL PROTECTED PRC [0xff02] _procedure_table
> 
> which clearly indicates how at least one non-GNU dynamic loader interprets 
> absolute symbols (the size of the procedure table, held in 
> `_procedure_table_size', surely is not supposed to be relocated by the 
> base address); for the record, `PRC [0xff02]' denotes the special 
> SHN_MIPS_TEXT section used to hold code in the MIPS/IRIX psABI in place of 
> the usual `.text' section.
> 
>  OK for this change with updated NEWS then?

OK with minor change in the NEWS entry (fixes the grammatical mistake of this->the).

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

>   Maciej
> 
> Changes from v1:
> 
> - NEWS entry added.
> ---
>  NEWS                                   |    7 +++++++
>  libc-abis                              |    2 ++
>  sysdeps/unix/sysv/linux/mips/libc-abis |    2 ++
>  3 files changed, 11 insertions(+)
> 
> glibc-abi-absolute.diff
> Index: glibc/NEWS
> ===================================================================
> --- glibc.orig/NEWS	2018-07-05 14:21:40.000000000 +0100
> +++ glibc/NEWS	2018-07-05 14:58:24.773218263 +0100
> @@ -9,6 +9,13 @@ Version 2.28
>  
>  Major new features:
>  
> +* The GNU C Library now has correct support for ABSOLUTE symbols
> +  (SHN_ABS-relative symbols).  Previously such ABSOLUTE symbols were
> +  relocated incorrectly or in some cases discarded.  The GNU linker can
> +  make use of this newer semantics, but it must communicate it to the

Suggest:
"make use of the corrected semantics, ..."

> +  dynamic loader by setting the ELF file's identification (EI_ABIVERSION
> +  field) to indicate such support is required.
> +
>  * Unicode 11.0.0 Support: Character encoding, character type info, and
>    transliteration tables are all updated to Unicode 11.0.0, using
>    generator scripts contributed by Mike FABIAN (Red Hat).
> Index: glibc/libc-abis
> ===================================================================
> --- glibc.orig/libc-abis	2018-06-29 16:46:11.558734836 +0100
> +++ glibc/libc-abis	2018-07-05 14:28:46.185297779 +0100
> @@ -46,3 +46,5 @@ IFUNC		powerpc64-*-linux*
>  IFUNC		powerpc-*-linux*
>  IFUNC		sparc64-*-linux*
>  IFUNC		sparc-*-linux*
> +# Absolute (SHN_ABS) symbols working correctly.
> +ABSOLUTE
> Index: glibc/sysdeps/unix/sysv/linux/mips/libc-abis
> ===================================================================
> --- glibc.orig/sysdeps/unix/sysv/linux/mips/libc-abis	2018-06-29 16:46:11.594994206 +0100
> +++ glibc/sysdeps/unix/sysv/linux/mips/libc-abis	2018-07-05 14:28:46.394860228 +0100
> @@ -14,3 +14,5 @@ UNIQUE
>  #
>  # MIPS O32 FP64
>  MIPS_O32_FP64   mips*-*-linux*
> +# Absolute (SHN_ABS) symbols working correctly.
> +ABSOLUTE
>
Maciej W. Rozycki - July 5, 2018, 5:09 p.m.
Hi Carlos,

> > +* The GNU C Library now has correct support for ABSOLUTE symbols
> > +  (SHN_ABS-relative symbols).  Previously such ABSOLUTE symbols were
> > +  relocated incorrectly or in some cases discarded.  The GNU linker can
> > +  make use of this newer semantics, but it must communicate it to the
> 
> Suggest:
> "make use of the corrected semantics, ..."

 Hmm, "semantics" is uncountable rather than plural[1], so I'm not sure if 
"this semantics" is actually a grammatical mistake (just like "Can I drink 
this milk?" is fine), but I'm fine with your suggestion.  I actually 
considered using "the" to avoid having this discussion. ;)

 Committed with your suggestion applied then.  Thank you for your review.

  Maciej

References:

[1] <https://www.oxfordlearnersdictionaries.com/definition/american_english/semantics>
Carlos O'Donell - July 5, 2018, 6:02 p.m.
On 07/05/2018 01:09 PM, Maciej W. Rozycki wrote:
> Hi Carlos,
> 
>>> +* The GNU C Library now has correct support for ABSOLUTE symbols
>>> +  (SHN_ABS-relative symbols).  Previously such ABSOLUTE symbols were
>>> +  relocated incorrectly or in some cases discarded.  The GNU linker can
>>> +  make use of this newer semantics, but it must communicate it to the
>>
>> Suggest:
>> "make use of the corrected semantics, ..."
> 
>  Hmm, "semantics" is uncountable rather than plural[1], so I'm not sure if 
> "this semantics" is actually a grammatical mistake (just like "Can I drink 
> this milk?" is fine), but I'm fine with your suggestion.  I actually 
> considered using "the" to avoid having this discussion. ;)

The use of 'the' with countable nouns is allowed when they refer to a
specific example, like in this case, and I find English speakers prefer
this form.

You are correct though, it's not a grammatical mistake to use 'this'
with the uncounted noun, I had forgotten about this completely.
Thank you for elightening me! :-)

>  Committed with your suggestion applied then.  Thank you for your review.

My key motivation was to avoid the use of "newer" since this temporal
reference becomes difficult to understand at a later time and
"corrected" is, well, always correct.

Patch

Index: glibc/NEWS
===================================================================
--- glibc.orig/NEWS	2018-07-05 14:21:40.000000000 +0100
+++ glibc/NEWS	2018-07-05 14:58:24.773218263 +0100
@@ -9,6 +9,13 @@  Version 2.28
 
 Major new features:
 
+* The GNU C Library now has correct support for ABSOLUTE symbols
+  (SHN_ABS-relative symbols).  Previously such ABSOLUTE symbols were
+  relocated incorrectly or in some cases discarded.  The GNU linker can
+  make use of this newer semantics, but it must communicate it to the
+  dynamic loader by setting the ELF file's identification (EI_ABIVERSION
+  field) to indicate such support is required.
+
 * Unicode 11.0.0 Support: Character encoding, character type info, and
   transliteration tables are all updated to Unicode 11.0.0, using
   generator scripts contributed by Mike FABIAN (Red Hat).
Index: glibc/libc-abis
===================================================================
--- glibc.orig/libc-abis	2018-06-29 16:46:11.558734836 +0100
+++ glibc/libc-abis	2018-07-05 14:28:46.185297779 +0100
@@ -46,3 +46,5 @@  IFUNC		powerpc64-*-linux*
 IFUNC		powerpc-*-linux*
 IFUNC		sparc64-*-linux*
 IFUNC		sparc-*-linux*
+# Absolute (SHN_ABS) symbols working correctly.
+ABSOLUTE
Index: glibc/sysdeps/unix/sysv/linux/mips/libc-abis
===================================================================
--- glibc.orig/sysdeps/unix/sysv/linux/mips/libc-abis	2018-06-29 16:46:11.594994206 +0100
+++ glibc/sysdeps/unix/sysv/linux/mips/libc-abis	2018-07-05 14:28:46.394860228 +0100
@@ -14,3 +14,5 @@  UNIQUE
 #
 # MIPS O32 FP64
 MIPS_O32_FP64   mips*-*-linux*
+# Absolute (SHN_ABS) symbols working correctly.
+ABSOLUTE