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

[Carlos O'Donell <carlos at redhat dot com>]

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
> 


-- 
Cheers,
Carlos.